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

fix(argo-cd): Fix checksum/cmd-params and checksum/cm to prevent unnecessary diffs #3027

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

pgmgb
Copy link

@pgmgb pgmgb commented Nov 13, 2024

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@pgmgb pgmgb changed the title Fix/argo cd checksum fix(argo-cd): Fix checksum/cmd-params and checksum/cm to prevent unnecessary diffs Nov 13, 2024
@jmeridth
Copy link
Member

Thank you for your PR but can you please sign your commit(s) to satisfy DCO.

Also, how does focusing on the data section of the yaml only give us more precise checksum? By using the config maps entire yaml, we get the same effect, correct? The data portion is included and any changes in the data will result in a new value.

Thank you for explaining.

@pgmgb
Copy link
Author

pgmgb commented Nov 14, 2024

@jmeridth
_common.tpl will define argo-cd.labels which includes helm.sh/chart: {{ include "argo-cd.chart" .context }}. argocd-cm.yaml will use argo-cd.labels inside the metadata.labels. Therefore, every new helm chart release will end in a new sha256sum of argocd-cm.yaml, even the data part did not change.

Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

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

Your code can/might work but I think we can simplify it.
WDYT, @pgmgb ?

@mbevc1
Copy link
Collaborator

mbevc1 commented Nov 14, 2024

Thanks for your contribution and updates. Could you also make sure you sign your commits please?

@pgmgb pgmgb force-pushed the fix/argo-cd-checksum branch from f568502 to 7ce6a4f Compare November 15, 2024 08:23
@pgmgb
Copy link
Author

pgmgb commented Nov 15, 2024

@mbevc1
I tried as explained in Checks -> Rebase the branch but somehow it was not successful.

@mbevc1
Copy link
Collaborator

mbevc1 commented Nov 15, 2024

Your signature must be verified and valid as well:
image

@mkilchhofer
Copy link
Member

mkilchhofer commented Nov 15, 2024

@pgmgb you can click on the "Details" link on the failing DCO. There the Bot tells you how to fix it:
https://github.com/argoproj/argo-helm/pull/3027/checks?check_run_id=33031654657

Rebase the branch
If you have a local git environment and meet the criteria below, one option is to rebase the branch and add your Signed-off-by lines in the new commits. Please note that if others have already begun work based upon the commits in this branch, this solution will rewrite history and may cause serious issues for collaborators (described in the git documentation under "The Perils of Rebasing").

You should only do this if:

  • You are the only author of the commits in this branch
  • You are absolutely certain nobody else is doing any work based upon this branch
  • There are no empty commits in the branch (for example, a DCO Remediation Commit which was added using --allow-empty)

To add your Signed-off-by line to every commit in this branch:

  1. Ensure you have a local copy of your branch by checking out the pull request locally via command line.
  2. In your local branch, run: git rebase HEAD~8 --signoff
  3. Force push your changes to overwrite the branch: git push --force-with-lease origin fix/argo-cd-checksum

Your signature must be verified and valid as well: image

@mbevc1 I think we do not enforce this :) From my side I'd be okay we have a passing DCO

@mbevc1
Copy link
Collaborator

mbevc1 commented Nov 15, 2024

Ah, okay. Ignore me then, but look at your signatures as @mkilchhofer suggested 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants