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 k8s 1.22 #5228

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Sep 20, 2021

Description

@@ -40,7 +40,6 @@ operator-sdk create webhook [flags]
--plural string resource irregular plural form
--programmatic-validation if set, scaffold the validating webhook
--version string resource Version
--webhook-version string version of {Mutating,Validating}WebhookConfigurations to scaffold. Options: [v1, v1beta1] (default "v1")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flag was marked as deprecated and because of this the generate doc is removing when ihmo should not

Copy link
Member

Choose a reason for hiding this comment

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

But we do remove deprecated flags from the docs, right? Do we still need to have this since officially go/v3 still supports v1beta1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs are generated automatically (see: https://github.com/operator-framework/operator-sdk/blob/master/Makefile#L42). Then, we need to change/fix that. See: #5286

Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 This scares me. We're removing a flag which technically is NOT backwards compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Sigh, I just now read all of the comments.

@@ -47,7 +50,6 @@ operator-sdk create api [flags]

```
--controller if set, generate the controller without prompting the user (default true)
--crd-version string version of CustomResourceDefinition to scaffold. Options: [v1, v1beta1] (default "v1")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flag was marked as deprecated and because of this the generate doc is removing when ihmo should not

go.mod Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2021
@camilamacedo86 camilamacedo86 force-pushed the sdk-update branch 5 times, most recently from b2fc215 to 51450ad Compare October 2, 2021 11:57
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Oct 2, 2021

/hold cancel

It is go to be reviewed and merged now 👍

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2021
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Camila, this is really helpful. Just a few nits, else /lgtm

@@ -40,7 +40,6 @@ operator-sdk create webhook [flags]
--plural string resource irregular plural form
--programmatic-validation if set, scaffold the validating webhook
--version string resource Version
--webhook-version string version of {Mutating,Validating}WebhookConfigurations to scaffold. Options: [v1, v1beta1] (default "v1")
Copy link
Member

Choose a reason for hiding this comment

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

But we do remove deprecated flags from the docs, right? Do we still need to have this since officially go/v3 still supports v1beta1?

changelog/fragments/bump-1.22.yaml Outdated Show resolved Hide resolved
changelog/fragments/bump-1.22.yaml Outdated Show resolved Hide resolved
changelog/fragments/bump-1.22.yaml Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

hi @varshaprasad96,

But we do remove deprecated flags from the docs, right? Do we still need to have this since officially go/v3 still supports v1beta1?

The docs are generated automatically (see: https://github.com/operator-framework/operator-sdk/blob/master/Makefile#L42). Then, we need to change/fix that. See: #5286

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

The changelog has a couple typos and I reworded some of the migration snippet. The rest of the PR looks fine.

migration:
header: Upgrade K8s versions to use 1.22 (golang/v3)
body: >
Note that to ensure the backwords compatibility SDK tool will try to downgrade the versions used if you need to still scaffold the v1beta1 for CRDs and Webhooks to pusblish your solutions into the old cluster versions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that to ensure the backwords compatibility SDK tool will try to downgrade the versions used if you need to still scaffold the v1beta1 for CRDs and Webhooks to pusblish your solutions into the old cluster versions.
Note that to ensure the backwards compatibility SDK tool will try to downgrade the versions used if you need to still scaffold the v1beta1 for CRDs and Webhooks to publish your solutions into older cluster versions.

sigs.k8s.io/controller-runtime v0.10.0
```

2) Update your Makafile by
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2) Update your Makafile by
2) Update your Makefile by

changelog/fragments/bump-1.22.yaml Outdated Show resolved Hide resolved
changelog/fragments/bump-1.22.yaml Outdated Show resolved Hide resolved
Camila Macedo added 2 commits October 12, 2021 22:13
…en and controller-runtime

Signed-off-by: Camila Macedo <cmacedo@redhat.com>
Signed-off-by: Camila Macedo <cmacedo@redhat.com>
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2021
Signed-off-by: Camila Macedo <cmacedo@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Oct 12, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2021
@camilamacedo86 camilamacedo86 merged commit acbc148 into operator-framework:master Oct 12, 2021
@camilamacedo86 camilamacedo86 deleted the sdk-update branch October 12, 2021 22:13
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.

None yet

3 participants