Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cleanup generator logs (SC-842) #1318

Merged
merged 4 commits into from
Mar 14, 2022
Merged

cleanup generator logs (SC-842) #1318

merged 4 commits into from
Mar 14, 2022

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Mar 4, 2022 •
edited
Loading

Proposed Commit Message

Remove redundant generator logs

Summary:
- currently we log redundant data, which at a glance looks like ds-identify runs twice
- use return code for control flow rather than strings
- group templated code and add demarcation comments around it
- explicitly define generator log file

Additional Context

Initially wanted to remove redundant log messages, but made some other "cleanup" changes that should not change our behavior.

holmanb added 4 commits March 4, 2022 12:00
@holmanb
group template-generated code
ba43dea
@holmanb
we know the logfile before runtime
1c19e7c
@holmanb
Don't write duplicate logs
b4f0751 
This makes it look like ds-identify has been ran
twice and the second log shouldn't be necessary.

ds-identify rc=0
ds-identify _RET=found
@holmanb
use return codes for control flow rather than global variables
6901dd3
@holmanb holmanb changed the title (wip) cleanup generator logs, et al cleanup generator logs, et al Mar 7, 2022
@holmanb holmanb changed the title cleanup generator logs, et al cleanup generator logs (SC-842) Mar 7, 2022
@TheRealFalcon TheRealFalcon requested a review from blackboxsw March 7, 2022 20:22
TheRealFalcon
TheRealFalcon approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM, but @blackboxsw can you also take a look?

@TheRealFalcon TheRealFalcon merged commit ee5e128 into canonical:main Mar 14, 2022
This was referenced May 12, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRealFalcon TheRealFalcon TheRealFalcon approved these changes

@blackboxsw blackboxsw Awaiting requested review from blackboxsw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@holmanb @TheRealFalcon