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

feat: Add 1.26, 1.27 and 1.29 deprecations #504

Merged

Conversation

david-ortiz-saez
Copy link
Contributor

WHAT

This PR is adding the following deprecations.

Example

This has been tested with the resources included inside fixtures in a k8s v1.24 running cluster. Here is the output:

❯ ./bin/kubent -t 1.26
1:08PM INF >>> Kube No Trouble `kubent` <<<
1:08PM INF version dev (git sha dev)
1:08PM INF Initializing collectors and retrieving data
1:08PM INF Target K8s version is 1.26.0
1:08PM INF Retrieved 198 resources from collector name=Cluster
1:08PM INF Retrieved 7 resources from collector name="Helm v3"
1:08PM INF Loaded ruleset name=custom.rego.tmpl
1:08PM INF Loaded ruleset name=deprecated-1-16.rego
1:08PM INF Loaded ruleset name=deprecated-1-22.rego
1:08PM INF Loaded ruleset name=deprecated-1-25.rego
1:08PM INF Loaded ruleset name=deprecated-1-26.rego
1:08PM INF Loaded ruleset name=deprecated-1-27.rego
1:08PM INF Loaded ruleset name=deprecated-1-29.rego
1:08PM INF Loaded ruleset name=deprecated-future.rego
__________________________________________________________________________________________
>>> Deprecated APIs removed in 1.26 <<<
------------------------------------------------------------------------------------------
KIND                         NAMESPACE     NAME                    API_VERSION                            REPLACE_WITH (SINCE)
FlowSchema                   <undefined>   service-accounts-test   flowcontrol.apiserver.k8s.io/v1beta1   flowcontrol.apiserver.k8s.io/v1beta3 (1.26.0)
PriorityLevelConfiguration   <undefined>   workload-medium         flowcontrol.apiserver.k8s.io/v1beta1   flowcontrol.apiserver.k8s.io/v1beta3 (1.26.0)
__________________________________________________________________________________________
>>> Deprecated APIs removed in 1.27 <<<
------------------------------------------------------------------------------------------
KIND                 NAMESPACE   NAME              API_VERSION              REPLACE_WITH (SINCE)
CSIStorageCapacity   default     my-csi-capacity   storage.k8s.io/v1beta1   storage.k8s.io/v1 (1.24.0)

NOTE. There is an existing open PR with this purpose, but it has been stalled for 60 days.

@david-ortiz-saez
Copy link
Contributor Author

I see that Sonar failed, but it is only complaining about duplicated entries of flowcontrol.apiserver.k8s.io inside pkg/collector/cluster.go I think that's fine as it is the same as per example storage.k8s.io

@david-ortiz-saez
Copy link
Contributor Author

Could you kindly review @dark0dave, @stepanstipl?

@dark0dave dark0dave self-assigned this Aug 16, 2023
@dark0dave dark0dave self-requested a review August 16, 2023 21:17
@dark0dave
Copy link
Collaborator

dark0dave commented Aug 16, 2023

I am happy to approve, @stepanstipl can we get this merged?

Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 I'm happy to merge if the only failing test is the SonarCloud smell.

Many thanks for your effort @david-ortiz-saez 🎉 👍

Copy link
Collaborator

@dark0dave dark0dave left a comment

Choose a reason for hiding this comment

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

Please rebase but looks good to me as well.

@david-ortiz-saez david-ortiz-saez force-pushed the feat/add-126-127-129-deprecations branch from 95c10f4 to 6575617 Compare August 21, 2023 08:47
@david-ortiz-saez
Copy link
Contributor Author

hi @dark0dave, @stepanstipl, I've rebased with master, everything should be fine now. Thanks for taking a look!

@stepanstipl
Copy link
Contributor

Hi @david-ortiz-saez thanks 👍 , I see there are 2 tests failing, both minor cosmetic bits:

  • Couple of new lines at the end in several files 1
  • And we require code to be formatted according to go standards (go fmt) 2

Footnotes

  1. https://github.com/doitintl/kube-no-trouble/actions/runs/5924046263/job/16060898402?pr=504

  2. https://github.com/doitintl/kube-no-trouble/actions/runs/5924046263/job/16060898910?pr=504

@david-ortiz-saez
Copy link
Contributor Author

I think it is fixed now @stepanstipl. Thanks for the tip

@stepanstipl
Copy link
Contributor

stepanstipl commented Aug 21, 2023

Thanks @david-ortiz-saez - I was looking further into the last remaining issue, and it appears to be the rego file causing the trouble actually (apologies, I too quickly suggested it's the go code in my last comment):

$ opa fmt -d -l pkg/rules/rego/*                                                                                                0
pkg/rules/rego/deprecated-1-27.rego

$ opa fmt -d pkg/rules/rego/*                                                                                                   0
--- /var/folders/xq/17348xc93zx6hzxlcwt5kjtc0000gn/T/.opafmt666689532   2023-08-21 14:09:33.000000000 +0200
+++ /var/folders/xq/17348xc93zx6hzxlcwt5kjtc0000gn/T/.opafmt016151851   2023-08-21 14:09:33.000000000 +0200
@@ -21,9 +21,9 @@

 deprecated_api(kind, api_version) = api {
        deprecated_apis = {"CSIStorageCapacity": {
-                       "old": ["storage.k8s.io/v1beta1"],
-                       "new": "storage.k8s.io/v1",
-                       "since": "1.24",
+               "old": ["storage.k8s.io/v1beta1"],
+               "new": "storage.k8s.io/v1",
+               "since": "1.24",
        }}

        deprecated_apis[kind].old[_] == api_version

You should be able to fix the formatting by running opa fmt -w pkg/rules/rego/*.

@david-ortiz-saez
Copy link
Contributor Author

My bad @stepanstipl. I identified the file and modified it but I did it manually because I wasn't aware of the -w option in the opa fmt command.
Nevertheless, it is fixed now. Thanks for your patience :)

@stepanstipl
Copy link
Contributor

Hi @david-ortiz-saez thanks... I still see one of the tests failing 1 - it's about the <summary> part of the commit message should be capitalized, e.g.:

- feat: include new deprecations in rego files
+ feat: Include new deprecations in rego files

Almost there 😄, let's get this merged...

Footnotes

  1. https://github.com/doitintl/kube-no-trouble/actions/runs/5926212143/job/16067784870?pr=504

@david-ortiz-saez
Copy link
Contributor Author

david-ortiz-saez commented Aug 23, 2023

I understand @stepanstipl, but how should I fix it? It is enough to add a new silly commit following that pattern so semantic release is triggered or do I need to force-push all the changes again in one single commit following the pattern?

@stepanstipl
Copy link
Contributor

Hi @david-ortiz-saez, all commits are checked to follow the pattern, i.e. it's not enough to just add one commit at the end. Here's how I do this - interactive rebase, which lets you edit commit messages, i.e.:

# make sure your local master is up-to-date
git checkout master
git pull
# switch to PR branch
git checkout add-126-127-129-deprecations
# rebase
git rebase -i master

In the editor you'll change the beginning of commits you want to change to r (or reword) and save (don't edit the actuall commit messages yet), and then git will take you through the individual commits marked for reword and let you edit the messages.

You'll need to force-push after.

Hope this helps 🤞

@david-ortiz-saez david-ortiz-saez force-pushed the feat/add-126-127-129-deprecations branch 3 times, most recently from 7e6b0b9 to 6c39a9d Compare August 23, 2023 09:32
@david-ortiz-saez
Copy link
Contributor Author

david-ortiz-saez commented Aug 23, 2023

thanks for the tip @stepanstipl! Everything should be fine now (I hope)

@stepanstipl
Copy link
Contributor

stepanstipl commented Aug 23, 2023

Hi @david-ortiz-saez all the tests look good, great job! 👍

One - hopefully really last thing - we require all commits to be signed, more on that 12 (I typically recommend going with GPG). Feel free to let me know if you need any help with that. Cheers 🙏

Footnotes

  1. https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

  2. https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@david-ortiz-saez
Copy link
Contributor Author

Hi @stepanstipl, I had configured the signing, but somehow during last rebase they were lost?
I'm sorry but can you please guide me how can I sign the commits that are already there? When I execute git rebase -i master I get the following

pick 6c86763 dep: Docker Bump golang from 1.20-alpine3.17 to 1.21-alpine3.17
pick 2ec21a9 fix: Missing buildx added to ci
pick a509671 dep: Go Bump github.com/rs/zerolog from 1.29.0 to 1.30.0
pick 801d5d3 chore: Add brew installation method
pick 17b650a feat: Include new deprecations in rego files
pick a563508 feat: Include new elements in cluster collector
pick 2aaa8a1 test: Add tests for new deprecations
pick d053b19 style: Include new lines at end of files and rego fmt
pick 6c39a9d style: Fix space identation

When I forced pushed these changes, the UI told me that my branch was not updated with master and I clicked on rebase button on the UI, then the 4 initial commits disappeared, but the signature in my commits was also removed.

What should be the proper approach here?

@stepanstipl
Copy link
Contributor

Hi @david-ortiz-saez, ok if you have git configured to sign then it should be ok when you just do rebase, but I assume if you use the GH UI functionality, it's not signing the commits (sorry, not sure, I don't use it).

The actual steps might depend a bit on how is your repo set up (forked), but based on the "not updated" message you mentioned, I'm assuming you're rebasing on your fork, instead of this repo master.

You can try smth. like this:

git remote add upstream git@github.com:doitintl/kube-no-trouble.git
git fetch upstream
git rebase -i upstream/master

and push. 🤞

@david-ortiz-saez david-ortiz-saez force-pushed the feat/add-126-127-129-deprecations branch from 6c39a9d to 025e291 Compare August 23, 2023 12:58
@david-ortiz-saez
Copy link
Contributor Author

@stepanstipl I think this is the good one, I've modified the commits and force pushed, I see that the branch is updated and the commits are compliance and signed.
Many thanks for your help! I've learned a lot in the process :D

@stepanstipl stepanstipl merged commit e506263 into doitintl:master Aug 24, 2023
25 of 26 checks passed
@stepanstipl
Copy link
Contributor

All good, thanks a lot for getting this through @david-ortiz-saez! 🎉

@mkurde
Copy link

mkurde commented Sep 4, 2023

When can we expect the release of this PR?

@stepanstipl stepanstipl changed the title feat: add 1.26 1.27 1.29 deprecations feat: Add 1.26, 1.27 and 1.29 deprecations Sep 5, 2023
@stepanstipl stepanstipl added the feature New feature or request label Sep 5, 2023
@stepanstipl
Copy link
Contributor

I would recommend using the latest nightly for now 1, which has this in. But 🤞 soon (~week or so). We just want to look at a couple of other things on the list 2, but I would like to get one out relatively soon.

Footnotes

  1. https://github.com/doitintl/kube-no-trouble/releases/tag/nightly-0.7.0-51-gf6ebd85

  2. https://github.com/doitintl/kube-no-trouble/milestone/6

@mkurde
Copy link

mkurde commented Sep 15, 2023

I would recommend using the latest nightly for now 1, which has this in. But 🤞 soon (~week or so).

Great news. I saw in the nightly build that the 1.27 deprecation (storage api) was listed in "future" deprecations even when I select target version 1.27.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants