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

[v2.6] Release Epinio OOB #2126

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

pennyscissors
Copy link
Contributor

@pennyscissors pennyscissors commented Sep 29, 2022

This PR is to release the Epinio chart OOB on behalf of the Epinio team.
The chart was validated by QA and approved here: #1814 (comment)

@pennyscissors pennyscissors changed the base branch from dev-v2.6 to release-v2.6 September 29, 2022 19:05
release.yaml Outdated
Comment on lines 2 to 4
- 100.1.3+up1.2.5
longhorn-crd:
- 100.1.3+up1.2.5
- 100.1.3+up1.2.5
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@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.

From a charts packaging perspective, this PR seems good to me.

However, disclaimers:

  • Since this is a copy-paste of a package introduced in dev-v2.6, I haven't reviewed the content of the chart in this PR since I assume that that review has already been done by another reviewer with this PR in dev-v2.6. For example, I'm not blocking my approval on a review to the comment I have left on this PR regarding the upstream-version since I assume this was discussed on merging the initial PR to dev-v2.6.
  • I'm assuming that Epinio's QA has already validated the functionality of this chart.

Having made these disclaimers, I'm approving this PR. cc: @prachidamle @MKlimuszka let me know if I should merge or whether we need a deeper content review.

catalog.cattle.io/rancher-version: '>= 2.6.0-0 < 2.7.0-0'
catalog.cattle.io/release-name: epinio
catalog.cattle.io/type: cluster-tool
catalog.cattle.io/upstream-version: 19.0.3
Copy link
Contributor

@aiyengar2 aiyengar2 Sep 29, 2022

Choose a reason for hiding this comment

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

is this upstream version correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a question for the Epinio team. Maybe @andreas-kupries or @thehejik?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pennyscissors

If the 19.0.3 refers to the epinio chart version then this is wrong. Should be 1.2.1.
See also 100.0.0+up1.2.1 in the file name.

I see this bad version in the #1814 as well. I do not remember writing it, anymore. I suspect that this is a copy/paste error I did. I.e. that I copied the annotations from some example or existing chart and then missed editing this one to the correct version number.

Apologies for that.

As this (2126) is your PR I suspect that I will not be able to fix that, right ? (I.e. no permissions)

Choose a reason for hiding this comment

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

@andreas-kupries , I thought we were all good to release? I'm confused between this message and the Slack messages. Can we merge this as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that we were good to release. I got very much blind-sided by the issue above.

If a bogus upstream-version like the above is a blocker (*) then we are not good to release, and should fix that annotation in the modified chart. Should (can?) this be fixed in this PR ? Or should I create a PR fixing it and then @pennyscissors updates this here with it ?


(*) I do not know what Rancher (marketplace) considers a blocker vs not.

Copy link
Contributor

Choose a reason for hiding this comment

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

To work ahead a bit, PR #2135 is a dev-v2.6 PR which fixes the annotation in the chart, and only that.
I hope that making it was the right thing to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the call for whether the upstream version should be a blocker for release is up to the Epinio team!

From the charts side, we have approved the PR and are just waiting for your approval on the PR review @andreas-kupries before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have approved, although it was moot.

I think the call for whether the upstream version should be a blocker for release is up to the Epinio team!

My view was that with upstream-version controlling functionality in the Marketplace (something about downgrading), the severity of a mismatch between annotation and actual version would be something the Rancher team would impose on the chart writers.

In the end I decided to essentially go with "it is a blocker" and fix it now instead of in the indeterminate future.

Thanks to all who took part in the journey here, and helped.

@MKlimuszka MKlimuszka self-requested a review September 29, 2022 19:51
@MKlimuszka
Copy link

Tomas (QA) approved releasing it today on Slack

@prachidamle
Copy link
Member

@aiyengar2 @pennyscissors we should be reviewing the chart w.r.t chart packaging, not validation of content /functionality the chart is providing.
@andreas-kupries validation of the chart functionality and content would be the responsibility of Epinio team, we will only review that the chart packaging is valid to release.

@aiyengar2
Copy link
Contributor

Charts Checklist (built for v0.3.x charts-build-scripts)

Scripts Gist: https://gist.github.com/PennyScissors/70fac62fcbda2030415474bfea842143

Checkpoint 0: Validate release.yaml

Validation steps:

  • Each chart version in release.yaml DOES NOT modify an already released chart. If so, stop and modify the versions so that it releases a net-new chart.
  • Each chart version in release.yaml IS exactly 1 more patch version than the last released chart version. If not, stop and modify the versions so that it releases a net-new chart.

Checkpoint 1: Compare contents of assets/ to charts/

Validation steps:

  • Running make unzip to regenerate the charts/ from scratch, then git diff to check differences between assets/ and charts/ yields NO differences or innocuous differences.

IMPORTANT: Do not undo these changes for future steps since we want to keep the charts/ that match the current contents of assets!

Checkpoint 2: Compare assets against index.yaml

Validation steps:

  • The index.yaml file has an entry for each chart version.
  • The index.yaml entries for each chart matches the Chart.yaml for each chart.
  • Each chart has ALL required annotations
    • kube-version annotation
    • rancher-version annotation
    • permits-os annotation (indicates Windows and/or Linux)

Checkpoint 3: Logical Checks

Omitted, as described in #2126 (comment)

@aiyengar2 aiyengar2 merged commit c8e0f1b into rancher:release-v2.6 Oct 5, 2022
Copy link
Contributor

@andreas-kupries andreas-kupries left a comment

Choose a reason for hiding this comment

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

Approved.

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.

5 participants