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

Remove insights_core_gpg_check from worker config #104

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

andywaltlova
Copy link
Collaborator

@andywaltlova andywaltlova commented Nov 2, 2023

Fixes: #103

  • Improve logging
  • Fix or workaround
    • Workaround would be to remove gpg verification option, the problem is present only when --no-gpg option is specified but we still want to verify yaml

🤔 Does it make sense to allow insights_gpg_check: false and verify_yaml: true, if we don't verify insights-client authenticity how can we trust verification results?

But it seems that the playbook worker does the same (I mean that is why I wrote it same in our worker), why..

the problematic exception in insight-core is here (log from worker can be seen in the #103)

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (809a01a) 65.34% compared to head (ca7c536) 64.58%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   65.34%   64.58%   -0.76%     
==========================================
  Files           5        5              
  Lines         352      336      -16     
==========================================
- Hits          230      217      -13     
+ Misses        101       99       -2     
+ Partials       21       20       -1     
Flag Coverage Δ
go-1.19 64.58% <100.00%> (-0.76%) ⬇️
go-1.20 64.58% <100.00%> (-0.76%) ⬇️

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

Files Coverage Δ
src/runner.go 76.25% <100.00%> (+0.42%) ⬆️
src/util.go 72.44% <ø> (-1.34%) ⬇️

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

@andywaltlova andywaltlova changed the title Improve wording of logging Remove insights_core_gpg_check from worker config Nov 3, 2023
@andywaltlova
Copy link
Collaborator Author

Talked it with other folks, seems like it was only for dev/qa purposes in playbook worker and it's not used in any other cases, in our worker we did not use it at all (not even for dev purposes). Imho best option is to remove it altogether.

@andywaltlova andywaltlova marked this pull request as ready for review November 3, 2023 08:49
@andywaltlova andywaltlova requested a review from r0x0d November 3, 2023 08:49
@r0x0d
Copy link
Member

r0x0d commented Nov 6, 2023

🤔 Does it make sense to allow insights_gpg_check: false and verify_yaml: true, if we don't verify insights-client authenticity how can we trust verification results?

I think it would make things a bit more complicated, I like the simplicity we have right now to rely only on verify_yaml, which gives devs/qa the ability to skip the validation if necessary without having to change the code itself.

And for the user, I don't expect them using this option at all.

@andywaltlova andywaltlova merged commit ed055ae into oamg:main Nov 6, 2023
@r0x0d r0x0d mentioned this pull request Feb 28, 2024
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.

when verify_yaml:true and insights_core_gpg_check:false worker fails and doesn't execute signed script
3 participants