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

chore(back8sup): update to latest #553

Merged
merged 12 commits into from
Feb 21, 2022
Merged

Conversation

eyenx
Copy link
Member

@eyenx eyenx commented Feb 14, 2022

Description

Updates to latest version of back8sup

Issues

n/a

Changes

Seen here

Mainly change in CI where we now use go-semantic-release and CI shellcheck update to 1.1.0 from 0.1.0

This made us also put "$GENERATIONS" in quotes and disabling SC2012 from line 155 in src/back8sup.sh

No changes to the function of the script.

Checklist

  • I updated the version in Chart.yaml
  • I updated applicable README.md files using pre-commit run
  • I documented any high-level concepts I'm introducing in docs/
  • If I updated a dependency tool, or app, this PR contains a short summary of the changes I'm pulling
  • CI is currently green and this is ready for review
  • I am ready to test changes after they are applied and released

Signed-off-by: Toni Tauro [email protected]

@eyenx eyenx requested review from a team and tongpu as code owners February 14, 2022 11:51
@eyenx eyenx requested review from hairmare and removed request for a team February 14, 2022 11:51
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 14, 2022
Copy link
Contributor

@hairmare hairmare left a comment

Choose a reason for hiding this comment

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

see reopened tasks

@eyenx eyenx force-pushed the chore/back8sup-update-to-0.7.8 branch from a36016d to cc28cbe Compare February 14, 2022 11:58
@eyenx eyenx requested review from hairmare and removed request for tongpu and hairmare February 14, 2022 11:58
Copy link
Contributor

@hairmare hairmare left a comment

Choose a reason for hiding this comment

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

see repoened tasks

@eyenx eyenx force-pushed the chore/back8sup-update-to-0.7.8 branch from 8a7fbb2 to 35f8c6c Compare February 14, 2022 12:21
@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2022
@eyenx eyenx changed the title chore(back8sup/backup-apps): update to latest chore(back8su): update to latest Feb 14, 2022
@eyenx eyenx changed the title chore(back8su): update to latest chore(back8sup): update to latest Feb 14, 2022
@eyenx
Copy link
Member Author

eyenx commented Feb 14, 2022

pluto does not want me to have batch/v1beta1 in there

…/git/adfinis-charts hack/pluto.sh charts/back8sup                                                                                                                                                                                                                                          
NAME                    KIND      VERSION         REPLACEMENT   REMOVED   DEPRECATED
release-name-back8sup   CronJob   batch/v1beta1   batch/v1      false     true

…/git/adfinis-charts echo $?                                                                                                                                                                                                                                                                
2

@eyenx eyenx marked this pull request as draft February 14, 2022 12:51
@hairmare
Copy link
Contributor

this PR contains a short summary of the changes I'm pulling

The description needs to be updated to reflect what is actually in the PR. All the linked changes I looked at have been in the chart for ages. For example, the move to GHCR was in #152 from Jan 2021.

@eyenx eyenx closed this Feb 15, 2022
@hairmare
Copy link
Contributor

followup in back8sup so we stop tagging empty releases: adfinis/back8sup#36

@hairmare hairmare deleted the chore/back8sup-update-to-0.7.8 branch February 15, 2022 07:13
@eyenx eyenx restored the chore/back8sup-update-to-0.7.8 branch February 16, 2022 21:14
@eyenx eyenx reopened this Feb 16, 2022
@eyenx
Copy link
Member Author

eyenx commented Feb 16, 2022

well it does not work. Pluto is saying batch/v1beta1 is deprecated and stops the CI. The only way to force it to accept this is by either having

helm template $chart --api-versions batch/v1/CronJob | pluto detect -

or

helm template $chart | pluto detect -v k8s=v1.21 -

Not sure if pluto is the right tool to go for this, I was also looking a lot into kubepug but that tool isn't throwing any errors regarding batch/v1beta1/CronJob at all, which makes me think it does not work as intended.

Based on this, I'm expecting CI to fail on charts/openshift-etcd-backup too. Not on charts/rmd tho, as it only checks for Capabilities.APIVersion.Has "batch/v1" which is kinda wrong, as it isn't explictly checking for v1/CronJob to be there.

thoughts @hairmare ?

@eyenx
Copy link
Member Author

eyenx commented Feb 16, 2022

looks like batch/v1beta1 is really not found by kubepug issue#225

@hairmare
Copy link
Contributor

Pluto is saying batch/v1beta1 is deprecated

but it's true, batch/v1beta1 is indeed deprecated in v1.21+ and will be removed in v1.25+.

would this maybe work if we upgrade helm to the latest version or tell it which version of k8s we want to render for?

@hairmare
Copy link
Contributor

hairmare commented Feb 17, 2022

also, how long do we plan on supporting k8s <=1.20?

it's EOL in vanilla k8s so maybe we should just drop supporting it.

@eyenx
Copy link
Member Author

eyenx commented Feb 17, 2022

Pluto is saying batch/v1beta1 is deprecated

but it's true, batch/v1beta1 is indeed deprecated in v1.21+ and will be removed in v1.25+.

Yes it is true. PLUTO is totally right and the right tool for this.

would this maybe work if we upgrade helm to the latest version or tell it which version of k8s we want to render for?

I don't think helm updating would help. It's how the templating works in this special usecase. We would need to give the templating additional arguments like --api-versions but I'm against that. Makes it too complicated, too many moving parts

also, how long do we plan on supporting k8s <=1.20?

it's EOL in vanilla k8s so maybe we should just drop supporting it.

This is in fact a good point. I would consider adding a TARGET_K8S_VERSION variable with the kubernetes version we are testing against? or what do you think on this behalf?

@hairmare
Copy link
Contributor

I don't think helm updating would help

My theory is that it needs to decide what capabilities a chart has based on some version of k8s that helm knows about.

We would need to give the templating additional arguments like --api-versions

In this case it would be --kube-version, i'm not sure which version is the default for our helm version but it might be a modern enough one with an updated helm install.

I would consider adding a TARGET_K8S_VERSION variable with the kubernetes version we are testing against? or what do you think on this behalf?

One way to dox the supported k8s version is by adding kubeVersion to Chart.yaml. Maybe Helm even takes this into account when templating (instead of wanting --kube-version).

@eyenx
Copy link
Member Author

eyenx commented Feb 17, 2022

The approach is good, but the chart does not work like that, as we explicitly ask for Capabilities:

…/charts/back8sup helm version                                                                            
version.BuildInfo{Version:"v3.8.0", GitCommit:"d14138609b01886f544b2025f5000351c9eb092e", GitTreeState:"clean", GoVersion:"go1.17.6"}
…/charts/back8sup grep kubeVersion Chart.yaml                                                            
kubeVersion: ">1.21.0"
…/charts/back8sup helm template . | grep beta1                                                           
apiVersion: batch/v1beta1
…/charts/back8sup helm template . --kube-version 1.23.3 | grep beta1                                         
apiVersion: batch/v1beta1
…/charts/back8sup helm template . --kube-version 1.19.9 | grep beta1                                         
Error: chart requires kubeVersion: >1.21.0 which is incompatible with Kubernetes v1.19.9

Use --debug flag to render out invalid YAML

I suggest adding kubeVersion to Chart.yaml, removing the "Capabilities" switch in CronJob.yaml

@eyenx
Copy link
Member Author

eyenx commented Feb 17, 2022

On the other hand, why are we asking for Capabilities and not directly having a kubeVersion switch in the Chart?

Ah.. I think it's because ArgoCD does it that way without defining Kuberentes Version.

@github-actions github-actions bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 17, 2022
@eyenx
Copy link
Member Author

eyenx commented Feb 17, 2022

@hairmare
Copy link
Contributor

The approach is good, but the chart does not work like that, as we explicitly ask for Capabilities:

the flag is doxed as influencing Capabilities:

      --kube-version string          Kubernetes version used for Capabilities.KubeVersion

@eyenx
Copy link
Member Author

eyenx commented Feb 17, 2022

yes but we ask for Capabilities.Has and no Kubeversion. this is why I changed it now.

@eyenx eyenx marked this pull request as ready for review February 17, 2022 22:04
@eyenx eyenx requested a review from hairmare February 18, 2022 12:15
@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 18, 2022
@hairmare hairmare enabled auto-merge (squash) February 21, 2022 09:58
@hairmare hairmare merged commit 79bd381 into master Feb 21, 2022
@hairmare hairmare deleted the chore/back8sup-update-to-0.7.8 branch February 21, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants