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

Add rebase script #1079

Closed

Conversation

aiyengar2
Copy link
Contributor

@aiyengar2 aiyengar2 commented Apr 5, 2021

This script powered #1072, which rebased 5 charts.

Related Issue: rancher/charts-build-scripts#31

@cbron
Copy link
Contributor

cbron commented Apr 5, 2021

Waiting to review until 2.5.8 calms down

@aiyengar2 aiyengar2 changed the base branch from dev-v2.5-source to dev-v2.5 May 17, 2021 22:07
@aiyengar2 aiyengar2 changed the base branch from dev-v2.5 to dev-v2.5-source May 17, 2021 22:10
Copy link
Contributor Author

@aiyengar2 aiyengar2 left a comment

Choose a reason for hiding this comment

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

This PR needs to be moved to the dev-v2.5 branch as the dev-v2.5-source branch is now deprecated.

The easiest way to do this would be to create a new local branch from the dev-v2.5 branch, cherry-pick your commits from the branch this PR points to to that new branch, and then run make charts to generate the charts/ and assets/ as part of the new provisioning workflow.

More details on the new workflow can be found in https://github.com/rancher/charts/blob/dev-v2.5/packages/README.md#common-workflow. Feel free to find me on Slack / RocketChat or email me at arvind.iyengar@suse.com for any questions.

@cbron
Copy link
Contributor

cbron commented May 18, 2021

Isn't this one closed now ? I thought we removed

@aiyengar2 aiyengar2 changed the base branch from dev-v2.5-source to dev-v2.5 May 18, 2021 20:55
@aiyengar2 aiyengar2 force-pushed the add_rebase_script_to_charts branch from 7442ed9 to 68e47fa Compare May 18, 2021 20:57
@aiyengar2
Copy link
Contributor Author

Updated base to dev-v2.5

@cbron
Copy link
Contributor

cbron commented May 26, 2021

Is this still a thing ? 😆

@aiyengar2
Copy link
Contributor Author

Is this still a thing ? 😆

Yup, still should be valid. Going to be using it soon for rebasing Monitoring

@aiyengar2
Copy link
Contributor Author

Pushed some modifications to this script due to needing to have support for rebasing charts that have an excludes directory.

@aiyengar2 aiyengar2 force-pushed the add_rebase_script_to_charts branch from 412a1bb to 8fad16a Compare June 10, 2021 22:33
@cbron cbron requested review from jiaqiluo and removed request for cbron October 4, 2021 15:13
else
echo "1) Add all changes to ${DEST_DIRECTORY} to staging (e.g. git add)"
fi
echo "2) Commit all other changes to save them (e.g. 'git add <changes>; git commit -m \"message\"')"
Copy link
Member

Choose a reason for hiding this comment

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

When I used this script, I did not do git commit to commit changes; instead, I just did the git add then exited the shell. So the git commit looks like an optional step here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, this seems to be improperly worded. The idea is that the script supports if you make additional changes during commits, as long as those changes were not made to packages/{PACKAGE}/charts, packages/{PACKAGE}/charts-crd, packages/{PACKAGE}/generated-changes/excludes. But that doesn't seem to be clearly communicated at all here 😅

In newer versions of Git, these arguments cannot be provided due to git/git@9a3e3ca.

Relevant comment:

> It could be argued that this change might break existing users.  I'd argue that those existing users are already broken, and they just don't know it.  Let them know that they're broken.
@aiyengar2 aiyengar2 mentioned this pull request Jun 9, 2022
@aiyengar2
Copy link
Contributor Author

Closing in favor of rebased PR in #1936

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.

6 participants