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

Changes for v0.2.0 #38

Merged
merged 16 commits into from
May 17, 2021
Merged

Changes for v0.2.0 #38

merged 16 commits into from
May 17, 2021

Conversation

aiyengar2
Copy link
Contributor

@aiyengar2 aiyengar2 commented May 5, 2021

Highlights:

  • Cleaned up code for unused stuff and created some helper functions to reduce redundant code
  • Removed make sync, make rebase
  • Renamed make docs to make template
  • Heavily simplified make validate and removed a ton of files that were overly complex due to new requirements. See below for more details
  • Modified scripts and templates to document the new features
  • Updated versioning for semver compliance for Helm 3.5.2
  • Added a package-ci script that can be used to validate that charts-build-script changes don't break basic functionality
  • Minor bugfixes

Related Issue: rancher/rancher#32199

@aiyengar2 aiyengar2 force-pushed the v0.2.0 branch 26 times, most recently from dc92712 to 5c41358 Compare May 8, 2021 01: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.

Lots of changes, here are the highlights!

@aiyengar2 aiyengar2 force-pushed the v0.2.0 branch 3 times, most recently from 9364c98 to 9f29f29 Compare May 8, 2021 02:09
@aiyengar2 aiyengar2 force-pushed the v0.2.0 branch 4 times, most recently from 22e9698 to f554c96 Compare May 10, 2021 22:24
@aiyengar2
Copy link
Contributor Author

FYI: while this doesn't directly solve #6, since we are committing the charts/, assets/ and index.yaml in the Staging Branch as part of the PR itself, any time a .patch file is generated will be caught during the PR itself.

Therefore, I think this change addresses the underlying issue even if there is no actual changes being introduced to the build scripts on prepare.

Copy link
Member

@jiaqiluo jiaqiluo left a comment

Choose a reason for hiding this comment

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

@aiyengar2
Awesome work! I cannot wait to use this new version of charts-build-scripts!

The PR looks good to me overall. I reviewed all changes but did not run the code.

Co-authored-by: Jiaqi Luo <6218999+jiaqiluo@users.noreply.github.com>
Copy link
Member

@jiaqiluo jiaqiluo left a comment

Choose a reason for hiding this comment

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

LGTM

@aiyengar2 aiyengar2 requested a review from StrongMonkey May 14, 2021 00:45

### Porting over Charts / Assets from another Branch

In the Staging branch, porting over charts from another branch (e.g. `dev-v2.x+1`) requires you to copy the contents of that branch into your Staging branch, which can be done with the following simple Bash script. However, you will need to manually regenerate the Helm index since you only want the index.yaml on the Staging branch to be updated to include the new chart.
Copy link

Choose a reason for hiding this comment

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

I'm a little confused on this part. Aren't dev and staging the same now ? Meaning isn't dev-v2.5 the staging branch ? The goal of this script I'm guessing is to move across release lines, not entirely clear.
As for the whole bash script below, can't you just copy paste a folder from one versions branch to another ? Script below is not intuitive, I don't get the point of the forked branch / port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah dev and staging are the same now. The goal of this script is, let's say, moving from dev-v2.6 to dev-v2.5.

You don't want to necessarily maintain two versions of the chart in packages/, so you can make the commit to dev-v2.6 with the packages/ changes and simply port the charts, assets, and index.yaml over to dev-v2.5.

Or, if we use @nickgerace 's idea of having a single dev-v2.x branch that contains packages, this would be the normal process to make that version available in dev-v2.5 or dev-v2.6 branches (e.g. identical to what we do today with making a commit to master and then porting it to release/v2.5 and release/v2.6.

Copying and pasting a folder isn't sufficient since you also need to regenerate the Helm index. But basic idea is the same yeah; you need to copy the charts/, the assets/ and regenerate the Helm index to port over just the generated resources.

You can't do a simple cherry-pick across branches since the index.yaml may have conflicts.

Copy link

Choose a reason for hiding this comment

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

No cherry-pick makes sense, as does index.yaml regeneration. But why not just copy package/logging folder, swap branches, and then run make charts and make validate again ?

Copy link
Contributor Author

@aiyengar2 aiyengar2 May 14, 2021

Choose a reason for hiding this comment

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

You’ll be committing 2 versions of the logging chart. This is fine, as long as you keep both copies up to date, but if all you want is to make the asset available it makes more sense to have a single source of truth.

e.g. let’s say you miss a file change in one of them. It’s possible you could end up with two charts with different contents and the same version number.

@cbron
Copy link

cbron commented May 14, 2021

I think the staging/prod branches look good, thanks for doing this.
I do see another small problem, fleet has 5000 as patch version: https://github.com/aiyengar2/charts/tree/bump_staging_v0_2_0/assets/fleet

@aiyengar2
Copy link
Contributor Author

I do see another small problem, fleet has 5000 as patch version:

That's expected right? 5 is the patch and 000 is the packageVersion. Discussing this in #38 (comment).

@aiyengar2 aiyengar2 requested a review from cbron May 14, 2021 01:17
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.

I do see another small problem, fleet has 5000 as patch version:

That's expected right? 5 is the patch and 000 is the packageVersion. Discussing this in #38 (comment).

Pushed change to switch to 2 digit packageVersions, this should now be 500

@aiyengar2 aiyengar2 merged commit cae97e4 into rancher:master May 17, 2021
Copy link

@cbron cbron left a comment

Choose a reason for hiding this comment

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

Works for now, still need to figure out how to manage multiple versions in future

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