Skip to content

Azure: refactor report ready code#422

Closed
johnsonshi wants to merge 6 commits into
canonical:masterfrom
johnsonshi:johnsonshi/refactor-azure-report-ready-code
Closed

Azure: refactor report ready code#422
johnsonshi wants to merge 6 commits into
canonical:masterfrom
johnsonshi:johnsonshi/refactor-azure-report-ready-code

Conversation

@johnsonshi
Copy link
Copy Markdown
Contributor

This PR refactors Azure report ready code to
include more robust tests and telemetry.

johnsonshi and others added 3 commits June 9, 2020 02:59
@johnsonshi
Copy link
Copy Markdown
Contributor Author

Do I need to add my name to the lp-to-git-user file? My launchpad profile is https://launchpad.net/~johnsonshi

@johnsonshi johnsonshi changed the title Refactor Azure report ready code. Azure: refactor report ready code Jun 9, 2020
@OddBloke OddBloke self-assigned this Jun 9, 2020
@johnsonshi
Copy link
Copy Markdown
Contributor Author

Thanks @OddBloke. I've added my username to tools/.github-cla-signers file.

Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Hi @johnsonshi, thanks for the submission, I'm strongly in favour of making our codebase more robust!

In that vein, this is quite a large pull request, and it looks to me like it probably could be split into multiple, smaller ones. This would make it easier to review, and more likely that I will notice issues during the review, therefore also serving to make our codebase more robust.

I'll leave it to you as the author to decide what the best way of breaking this up is, but if you would like a suggestion: it seems to me that the changes to add more report_diagnostic_event and logging calls to existing code could quite easily be separated from the more complex goal state-related changes; that would at least give us two smaller chunks to work through.

Let me know if you have any questions!

Comment on lines +410 to +411
LOG.warning(msg)
report_diagnostic_event(msg)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It looks like we're logging alongside ~every report_diagnostic_event call; I wonder if it might make more sense to change the signature of the function to report_diagnostic_event(msg, log_func) and then have, for example, this call look like the below?

Suggested change
LOG.warning(msg)
report_diagnostic_event(msg)
report_diagnostic_event(msg, LOG.warning)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a very good suggestion @OddBloke! I'll implement that in the separate PR that will focus on increased logging/diagnostic reports.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion on how to split up the PR. I'll split it up so that 1 is goal state related and the other one is logging/telemetry related.

@OddBloke
Copy link
Copy Markdown
Collaborator

OddBloke commented Jun 9, 2020 via email

@OddBloke
Copy link
Copy Markdown
Collaborator

I'm going to close this out, as it's going to be reworked substantially (and I keep clicking on it to figure out if anything has changed 😁). Feel free to reopen it, or submit new PRs once you're ready. Thanks!

@OddBloke OddBloke closed this Jun 19, 2020
@johnsonshi johnsonshi deleted the johnsonshi/refactor-azure-report-ready-code branch August 14, 2020 01:26
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.

2 participants