Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Update CRD version #599

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Update CRD version #599

merged 2 commits into from
Jul 6, 2021

Conversation

phillebaba
Copy link
Member

This change will update the CRD version from v1beta1 to v1 in preparation for k8s v1.22. I have documented the tests that I have run to determine the different behaviors of helm-operator in different scenarios. Hopefully it is easy to follow along and understand what has been tested.

paths=./pkg/apis/...
mv ${CRD_DIR}/helm.fluxcd.io_helmreleases.yaml ${CRD_DIR}/helmrelease.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a weird behavior that the file will be different when the crd version is set. I do not know why this happens and I could not figure out how to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

I think an option may be to use output:stdout (instead of output:dir=...) and then write it directly to the file, instead of moving one around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip, makes it a lot cleaner. I did however have to trim the first two lines of the generated CRD as the new line and ---caused other issues.

@@ -1,24 +1,27 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

Is this some code gen error? We have an empty yaml doc then v1beta1 CRD, shouldn't it be v1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch this issue was caused by some extra new lines in the crd generation. Should be fixed now by removing them.

@phillebaba
Copy link
Member Author

I do not have access to the Circle CI tests so I cant see why the tests are failing. Running make testlocally works so that may not be the issue.

@hiddeco
Copy link
Member

hiddeco commented Apr 8, 2021

The whole end-to-end test suite fails due to the operator not installing correctly.

@phillebaba
Copy link
Member Author

Ah missed that, looks like I should be able to run e2e tests locally to debug this?

@hiddeco
Copy link
Member

hiddeco commented Apr 8, 2021

Should work with make e2e.

@phillebaba
Copy link
Member Author

@hiddeco so the E2E tests works now. I was looking at the totally wrong location, the issue was never in the tests but rather that values were not being parsed properly. Had to understand the tests before I could understand that. Can you have a look now?

Signed-off-by: Philip Laine <philip.laine@gmail.com>
Signed-off-by: Kingdon Barrett <yebyen@gmail.com>
This was referenced Jun 23, 2021
@kingdonb
Copy link
Member

kingdonb commented Jul 6, 2021

I incorporated this PR in #623 and will rebase it now manually to match that, so it closes out when that one is merged.

@kingdonb
Copy link
Member

kingdonb commented Jul 6, 2021

It rebased cleanly 👍

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thank you @phillebaba 🍻, and thanks @kingdonb for driving the release 🚋

@kingdonb kingdonb merged commit abb79a3 into fluxcd:master Jul 6, 2021
@kingdonb kingdonb mentioned this pull request Jul 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants