Skip to content

Conversation

@rane-rajasi
Copy link
Contributor

@rane-rajasi rane-rajasi commented Oct 6, 2021

  • Also, logging contents every time we write to a file (for improved telemetry), only from handler in this change

Q:

  • Should we also update the status file for non-enable commands after the execution is complete? -> No, for this PR, since we are not writing status for non-enable commands
  • Should we add a message in status file on which command led to the creation/update of status file -> No, because of the answer above

@rane-rajasi rane-rajasi requested review from a team, benank and kjohn-msft and removed request for a team October 6, 2021 15:07
@kjohn-msft
Copy link
Collaborator

For later (separate task): [For now, don't write status for non-enable]

  1. Do not fail non-enable calls on anything that is succeeding today. (That would be a regression) - sequence number discovery and status writes are best effort for non-enable calls.
  2. Do write end status for non-enable commands.

@kjohn-msft
Copy link
Collaborator

Review improvement based on when this goes in. See if this is sufficient or whether it needs to happen even faster.

Copy link
Collaborator

@kjohn-msft kjohn-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments inline

Comment on lines 82 to 83
status_json = self.ext_output_status_handler.read_file(seq_no)
if status_json is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert to basic file system exists check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 76 to 77
seq_no = self.ext_config_settings_handler.get_seq_no()
if seq_no is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only check env var for lightweight check (as discussed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

file_path = os.path.join(dir_path, file_name)
error_message = ""
self.logger.log("Writing file. [File={0}]".format(file_name))
self.logger.log("Writing file. [File={0}] [Content={1}]".format(file_name, str(content)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing json file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def mock_get_all_versions_exception(self, extension_pardir):
raise Exception

def mock_getenv(self, is_enable_request=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@kjohn-msft kjohn-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments inline

@rane-rajasi rane-rajasi merged commit cccd8c0 into master Oct 7, 2021
@rane-rajasi rane-rajasi deleted the rarane-statusupdate branch January 5, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants