8000 cli: debug: various improvements by Nirusu · Pull Request #995 · edgelesssys/constellation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cli: debug: various improvements #995

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
Merged
merged 21 commits into from
Jan 18, 2023
Merged

cli: debug: various improvements #995

merged 21 commits into from
Jan 18, 2023

Conversation

Nirusu
Copy link
Contributor
@Nirusu Nirusu commented Jan 17, 2023

Proposed change(s)

  • Minor style changes (fix typos, fill values, config everywhere instead of switching between config and configuration)
  • Remove ambiguous log messages
  • Don't print service account uri (while it could be useful for debugging, it is full of secrets)
  • Print parsed arguments only if parsing did not fail (they will be the default value anyways otherwise)
  • Add debug logs for errors encountered in retry loops
  • Disable spinner for other debug logged commands than just create (mini up, create)

Nothing truly groundbreaking, not sure if change log worthy but I haven't tagged it for now.

(Also more of a refactor than a fix so ignore the branch naming)

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Link to Milestone

@Nirusu Nirusu requested a review from katexochen as a code owner January 17, 2023 09:48
@edgelesssys edgelesssys deleted a comment from netlify bot Jan 17, 2023
katexochen
katexochen requested changes Jan 17, 2023
View reviewed changes
Copy link
Member
@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

Will you squash this PR? If not I'd suggest to at least squash some of the commits together...

8000
@Nirusu Nirusu requested a review from derpsteb as a code owner January 17, 2023 16:09
@Nirusu Nirusu requested a review from katexochen January 17, 2023 16:09
katexochen
katexochen reviewed Jan 17, 2023
View reviewed changes
@Nirusu
Copy link
Contributor Author
Nirusu commented Jan 17, 2023

Alright, I went a bit loco with the %s -> %q replacement but I think it looks good for the most part and gives a distinction whether a string is hard-coded or a variable in a debug message. This also went into other components so double-check if that's fine, please. I kinda went hardcore with manual search & replace.

@katexochen
Copy link
Member

I kinda went hardcore with manual search & replace.

Yes, that was a little to much maybe. There are indeed situations where it isn't needed/wanted to use %q. The versionsapi uses the %q where need. I'd suggest to only replace %s with %q when handling things like user input for path etc. Also please don't replace %s with %v. You can also print errors with %s without calling err.Error().

@Nirusu
< 8000 span class="Button-label"> Copy link
Contributor Author
Nirusu commented Jan 17, 2023

I'll just limit this to flag parsing and user entry from the config then. That should hopefully be a clear enough distinction.

@Nirusu Nirusu requested a review from katexochen January 17, 2023 17:17
@Nirusu
Copy link
Contributor Author
Nirusu commented Jan 17, 2023

Another attempt I guess. Revert commit is not 100% clean since other changes were already on top but I'll squash everything later anyway.

@Nirusu Nirusu removed the request for review from derpsteb January 17, 2023 17:29
katexochen
katexochen reviewed Jan 18, 2023
View reviewed changes
Copy link
Member
@katexochen katexochen left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Nirusu added 17 commits January 18, 2023 10:47
@Nirusu
cli: debug: falg -> flag
69e774b
@Nirusu
cli: debug: Actually print configPath
53534a9
@Nirusu
cli: debug: Don't print serviceAccountUri
43ccd05
@Nirusu
cli: debug: helm -> Helm
35074f0
@Nirusu
cli: debug: Print error from retrier
00886b3
@Nirusu
cli: debug: Print "received response" only when no error was encountered
583564b
@Nirusu
cli: debug: Disable spinner for debug in mini up and init too
6f7d224
@Nirusu
cli: debug: configuration -> config (for consistency)
8b97844
@Nirusu
cli: debug: don't log ownerID
954a494
@Nirusu
cli: fix typo autoscale flag -> conformance
d16a719
@Nirusu
cli: debug: don't print values when parsing failed
9c5b140
@Nirusu
cli: debug: don't leave secret path message blank at the end
9c62edc
@Nirusu
cli: debug: config -> configuration
f336850
@Nirusu
cli: debug: Use real wording for parseVerifyFlags
21e0079
@Nirusu
cli: debug: %s -> %q / %v
5ac011e
@Nirusu
cli: debug: add service account URI debug log message back but withou…
ede4581 
…t values
@Nirusu
cli: debug: node-endpoint flag
a61d58d
@Nirusu
Copy link
Contributor Author
Nirusu commented Jan 18, 2023

Haven't seen the last review comment. Yes, I'll squash the whole PR. Most changes are not notable enough for single commits.

@Nirusu Nirusu merged commit a3db3c8 into main Jan 18, 2023
@Nirusu Nirusu deleted the fix/debug-log branch January 18, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katexochen katexochen katexochen approved these changes

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
@Nirusu @katexochen
0