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

📖 Update extension upgrade tutorial doc #1469

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

trgeiger
Copy link
Contributor

@trgeiger trgeiger commented Nov 14, 2024

With the addition of the CRD Upgrade Safety preflight check, our existing example of upgrading argocd from 0.5.0 to 0.6.0 no longer serves as a good example. This commit changes the example to use an update from 0.2.0 to 0.2.1 which no longer fails the CRD Upgrade check, and also doesn't violate our restriction on automatic upgrades between minor versions with a major version of 0.

Addresses #1459

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024
Copy link

netlify bot commented Nov 14, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 9e49f8a
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/673cbc02f04e710008afa854
😎 Deploy Preview https://deploy-preview-1469--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.69%. Comparing base (5445800) to head (9e49f8a).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1469      +/-   ##
==========================================
- Coverage   74.73%   74.69%   -0.04%     
==========================================
  Files          42       42              
  Lines        3241     3241              
==========================================
- Hits         2422     2421       -1     
- Misses        646      647       +1     
  Partials      173      173              
Flag Coverage Δ
e2e 52.32% <ø> (-0.10%) ⬇️
unit 57.17% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@trgeiger trgeiger force-pushed the docs-1459 branch 2 times, most recently from 72d38af to 4713083 Compare November 15, 2024 19:40
With the addition of the CRD Upgrade Safety preflight check, our
existing example of upgrading argocd from 0.5.0 to 0.6.0 no longer
serves as a good example. This commit changes the example to use an
update from 0.2.0 to 0.2.1 which no longer fails the CRD Upgrade check,
and also doesn't violate our restriction on automatic upgrades between
minor versions with a major version of zero.

Signed-off-by: Tayler Geiger <[email protected]>
@trgeiger
Copy link
Contributor Author

Should we change the ClusterExtension sample argocd manifest to match our docs at version 0.2.0 or 0.2.1? Or is it good enough to make these docs examples just work and leave the manifest alone at 0.6.0?


```bash
# Update to v0.11.0
kubectl patch clusterextension argocd --type='merge' -p '{"spec": {"source": {"catalog": {"version": "0.11.0"}}}}'
kubectl patch clusterextension argocd --type='merge' -p '{"spec": {"source": {"catalog": {"version": "0.2.1"}}}}'
Copy link
Member

Choose a reason for hiding this comment

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

Will this work?

I believe https://raw.githubusercontent.com/operator-framework/operator-controller/main/config/samples/olm_v1_clusterextension.yaml is currently installing 0.6.0.

So if as a previous step we install 0.6.0 then here we effectively are trying to downgrade to 0.2.1.

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, that's why I have my comment/question above about if we should change the sample manifest. The existing example of using 0.11.0 doesn't work either because it's attempting to upgrade a minor version within a zero major version which we don't support.

This specific line in 'olmv1_getting_started.md' technically isn't part of any tutorial or anything, but rather a one-off example of what an upgrade command might look like. So I think we could get away with whatever we want here, although having it be functional and not show an example of non-functional minor version upgrade attempts would probably be best. I could rewrite it to not have any actual version in the command, maybe, but rather some placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m1kola @everettraven so given we are adding a 0.2.0 manifest to the upgrade tutorial, should we just leave this line in olmv1_getting_started the same at 0.11.0 or maybe some other change?

Copy link
Member

Choose a reason for hiding this comment

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

I don't like that we have duplication of content. Maybe we should consolidate the tutorial and getting started somehow?

docs/tutorials/upgrade-extension.md Outdated Show resolved Hide resolved
@trgeiger trgeiger force-pushed the docs-1459 branch 3 times, most recently from 6a07c32 to c0dcefa Compare November 18, 2024 20:36
@trgeiger trgeiger marked this pull request as ready for review November 18, 2024 20:37
@trgeiger trgeiger requested a review from a team as a code owner November 18, 2024 20:37
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024
@trgeiger
Copy link
Contributor Author

Admonition added at the beginning of the tutorial with the v0.2.0 manifests.

Admonition added at the very end explaining the possible discrepancy in last-applied-configuration depending on if you applied a new yaml file or used patch/edit to upgrade the ClusterExtension.

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Just a few comments. The "Upgrade an Extension" looks good to me. We need to sort out the "Getting Started" page, but we can do it in a separate PR, if you prefer.

docs/tutorials/upgrade-extension.md Outdated Show resolved Hide resolved
docs/tutorials/upgrade-extension.md Outdated Show resolved Hide resolved
docs/tutorials/upgrade-extension.md Outdated Show resolved Hide resolved

```bash
# Update to v0.11.0
kubectl patch clusterextension argocd --type='merge' -p '{"spec": {"source": {"catalog": {"version": "0.11.0"}}}}'
kubectl patch clusterextension argocd --type='merge' -p '{"spec": {"source": {"catalog": {"version": "0.2.1"}}}}'
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that we have duplication of content. Maybe we should consolidate the tutorial and getting started somehow?

@m1kola m1kola added documentation Improvements or additions to documentation kind/documentation Categorizes issue or PR as related to documentation. labels Nov 19, 2024
@trgeiger
Copy link
Contributor Author

I agree re: consolidating content. I actually opened #1480 to address that issue more broadly with duplicate content between How-To Guides and Tutorials. Maybe we can address this duplicate upgrade command when we hit that ticket.

@m1kola
Copy link
Member

m1kola commented Nov 19, 2024

@trgeiger makes senes. Should we revert / partially revert docs/getting-started/olmv1_getting_started.md here in that case? Or do you think it is still valuable to have these changes?

@trgeiger trgeiger force-pushed the docs-1459 branch 2 times, most recently from e20b9e2 to 6b59582 Compare November 19, 2024 16:00
For our examples to work, we need to use a very specific version of our
example operator. The upgrade from 0.2.0 to 0.2.1 functions properly for
our purposes, but we don't want to affect the sample operator in
config/samples so we are adding the example manifest for 0.2.0 directly
to this tutorial.

Signed-off-by: Tayler Geiger <[email protected]>
@m1kola m1kola added this pull request to the merge queue Nov 19, 2024
Merged via the queue into operator-framework:main with commit 6995bc4 Nov 19, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation kind/documentation Categorizes issue or PR as related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants