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.

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

Improve logging when insights_core_gpg_check is disabled #90

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Sep 25, 2023

If the user specify that the insights gpg check should be disabled, we should do nothing and execute the script anyway, as there is no gpg check to be performed. Otherwise, if the gpg check is still enabled, we call insights-client to verify the signature.

If there is no need to validate the gpg signature, we should not call insights-client for it.

HMS-2647

@r0x0d r0x0d requested a review from andywaltlova September 25, 2023 17:32
@r0x0d r0x0d self-assigned this Sep 25, 2023
@r0x0d r0x0d force-pushed the update-signature-logging branch 3 times, most recently from 18e59f2 to e05c994 Compare September 25, 2023 17:38
@r0x0d r0x0d changed the title Update yaml signature logging Improve logging when InsightsCoreGpgCheck is disabled Sep 25, 2023
@r0x0d r0x0d changed the title Improve logging when InsightsCoreGpgCheck is disabled Improve logging when insights_core_gpg_check is disabled Sep 25, 2023
Copy link
Collaborator

@andywaltlova andywaltlova left a comment

Choose a reason for hiding this comment

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

Have a question, based on the changes it's not clear to me why the gpg check even exists 😅 So we have basically four cases right?

  • verification enabled, gpg check enabled
    • insights client call without --no-gpg
  • verification enabled, gpg check disabled
    • this is my question - this should append the --no-gpg option to insights client verification call?
  • verification disabled, gpg enabled
    • verification and gpg should be skipped, no insights-client call
  • verification disabled, gpg disabled
    • verification simply disabled, no insights-client call

@r0x0d r0x0d force-pushed the update-signature-logging branch 3 times, most recently from 2599303 to 8a5f99e Compare September 28, 2023 14:14
Update the logs that related to the signature verification. Now it is
more clear to the user that the insights-client call will still occur,
but the result may be ignored because the validation is disabled.

Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d r0x0d force-pushed the update-signature-logging branch from 8a5f99e to a9f6a4c Compare September 28, 2023 14:15
@r0x0d r0x0d requested a review from andywaltlova September 28, 2023 14:15
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ef09111) 65.61% compared to head (a9f6a4c) 65.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   65.61%   65.34%   -0.28%     
==========================================
  Files           5        5              
  Lines         349      352       +3     
==========================================
+ Hits          229      230       +1     
- Misses        100      101       +1     
- Partials       20       21       +1     
Flag Coverage Δ
go-1.16 65.34% <72.72%> (-0.28%) ⬇️
go-1.20 65.34% <72.72%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/main.go 0.00% <ø> (ø)
src/runner.go 75.82% <72.72%> (-2.34%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@andywaltlova andywaltlova left a comment

Choose a reason for hiding this comment

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

Just one minor typo in log - suggested improvement

Co-authored-by: Andrea Waltlová <[email protected]>
@r0x0d r0x0d merged commit b7f2858 into oamg:main Sep 29, 2023
@r0x0d r0x0d deleted the update-signature-logging branch September 29, 2023 17:24
@r0x0d r0x0d mentioned this pull request Oct 16, 2023
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.

3 participants