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

Use webhook secrets from files #3177

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Apr 25, 2024

Summary

Instead of relying on the config to set them.

Relates to: https://github.com/stacklok/infra/issues/954

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

I just verified that the env names are correct by running:

MINDER_WEBHOOK_CONFIG_WEBHOOK_SECRET_FILE=.secrets/webhook_secret MINDER_WEBHOOK_CONFIG_PREVIOUS_WEBHOOK_SECRET_FILE=.secrets/previous_secrets make run-server

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@jhrozek
Copy link
Contributor Author

jhrozek commented Apr 25, 2024

Submitted as draft because we need to merge https://github.com/stacklok/infra/pull/1189 first

@coveralls
Copy link

coveralls commented Apr 25, 2024

Coverage Status

coverage: 50.164% (-0.02%) from 50.18%
when pulling e7bea2c on jhrozek:use_wh_secret_from_file
into b135a03 on stacklok:main.

@jhrozek jhrozek marked this pull request as ready for review April 25, 2024 13:24
@jhrozek jhrozek requested a review from a team as a code owner April 25, 2024 13:24
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Based on the values I see in infra, I think these are the right values

deployment/helm/templates/deployment.yaml Show resolved Hide resolved
deployment/helm/templates/deployment.yaml Show resolved Hide resolved
@jhrozek
Copy link
Contributor Author

jhrozek commented Apr 25, 2024

@eleftherias thank you for catching the bad names!

eleftherias
eleftherias previously approved these changes Apr 25, 2024
@JAORMX
Copy link
Contributor

JAORMX commented Apr 26, 2024

Are we good to merge this?

@jhrozek
Copy link
Contributor Author

jhrozek commented Apr 26, 2024

Are we good to merge this?

In general yes, I only didn't merge because I'm intermittently available today and I didn't want to force someone else to pick up the breakage in case this doesn't work.

Instead of relying on the config to set them.

Relates to: stacklok/infra#954
@jhrozek
Copy link
Contributor Author

jhrozek commented Apr 29, 2024

@eleftherias @JAORMX could you please approve once again? I just resolved a conflict, no other changes

@jhrozek jhrozek merged commit 0f68185 into mindersec:main Apr 29, 2024
20 checks passed
jhrozek added a commit to jhrozek/minder that referenced this pull request Apr 29, 2024
Here's what happened:
 - I submitted PR mindersec#3177
 - Ria submitted feedback inline, I accepted it. The suggestions were
   merged into the remote branch.
 - the patch had conflicts with main
 - I resolved the conflicts locally and force-pushed to the remote branch. My
   local branch didn't have Ria's suggestions and the force-push removed
   them

This patch brings back Ria's suggestions so I included her as a
co-author.

Sorry!

Co-authored-by: Eleftheria Stein-Kousathana <eleftheria@stacklok.com>
@jhrozek jhrozek mentioned this pull request Apr 29, 2024
10 tasks
jhrozek added a commit that referenced this pull request Apr 29, 2024
Here's what happened:
 - I submitted PR #3177
 - Ria submitted feedback inline, I accepted it. The suggestions were
   merged into the remote branch.
 - the patch had conflicts with main
 - I resolved the conflicts locally and force-pushed to the remote branch. My
   local branch didn't have Ria's suggestions and the force-push removed
   them

This patch brings back Ria's suggestions so I included her as a
co-author.

Sorry!

Co-authored-by: Eleftheria Stein-Kousathana <eleftheria@stacklok.com>
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.

4 participants