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

[4.2] addons: List cilium-init image only for Cilium < 1.6 #1397

Merged
merged 2 commits into from
Dec 11, 2020

Conversation

vadorovsky
Copy link
Contributor

We don't ship a separate cilium-init image anymore. The cilium-init
script is included in the main cilium container image.

Fixes: SUSE/avant-garde#1923

Signed-off-by: Michal Rostecki mrostecki@opensuse.org

Copy link

@soumynathan soumynathan 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.

Copy link
Contributor

@mmnelemane mmnelemane left a comment

Choose a reason for hiding this comment

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

Isnt it possible to plainly skip the test altogether if the version is above 1.6 instead of running a dummy string test ?

Copy link
Contributor

@mmnelemane mmnelemane left a comment

Choose a reason for hiding this comment

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

Is it possible to skip the call to GetCiliumInitImage() here instead of passing an empty string ?

@vadorovsky
Copy link
Contributor Author

vadorovsky commented Oct 15, 2020

Isnt it possible to plainly skip the test altogether if the version is above 1.6 instead of running a dummy string test ?

What do you mean?

Is it possible to skip the call to GetCiliumInitImage() here instead of passing an empty string ?

Not really. Look here:

func init() {
registerAddon(kubernetes.Cilium, renderCiliumTemplate, renderCiliumPreflightTemplate, ciliumCallbacks{}, normalPriority, []getImageCallback{GetCiliumInitImage, GetCiliumOperatorImage, GetCiliumImage})
}

[]getImageCallback{GetCiliumInitImage, GetCiliumOperatorImage, GetCiliumImage}

The registerAddon function expects the full slice with getImageCallback functions. Since it's called in the init function, you cannot pass any arguments there, so you can't determine the version when callbacks are registered.

Honesly, I don't think there is any better way of handling different versions without a huge rewrite of the whole addons mechanism, but I'm open for suggestions.

Also, keep in mind that this is a backport and the same change is already merged in master (#1396). So if there is any other way of handling the <1.6 case which would be better for you, we would need to push it to master first.

@mmnelemane
Copy link
Contributor

Isnt it possible to plainly skip the test altogether if the version is above 1.6 instead of running a dummy string test ?

What do you mean?

Is it possible to skip the call to GetCiliumInitImage() here instead of passing an empty string ?

Not really. Look here:

func init() {
registerAddon(kubernetes.Cilium, renderCiliumTemplate, renderCiliumPreflightTemplate, ciliumCallbacks{}, normalPriority, []getImageCallback{GetCiliumInitImage, GetCiliumOperatorImage, GetCiliumImage})
}

[]getImageCallback{GetCiliumInitImage, GetCiliumOperatorImage, GetCiliumImage}

The registerAddon function expects the full slice with getImageCallback functions. Since it's called in the init function, you cannot pass any arguments there, so you can't determine the version when callbacks are registered.

Honesly, I don't think there is any better way of handling different versions without a huge rewrite of the whole addons mechanism, but I'm open for suggestions.

Also, keep in mind that this is a backport and the same change is already merged in master (#1396). So if there is any other way of handling the <1.6 case which would be better for you, we would need to push it to master first.

Sorry, I was only referring to the test code here: https://github.com/SUSE/skuba/pull/1397/files#diff-5f300cc81d08e60d6fa85478fa933ba35c084d1d3c2a7b31f5fdc12ac075dda1R50. where you are passing an empty string ? Again, its not very crucial.. just a suggestion. Otherwise Looks Good to me.

mmnelemane
mmnelemane previously approved these changes Oct 15, 2020
Add the versioncheck module with VersionCompare function.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
We don't ship a separate cilium-init image anymore. The cilium-init
script is included in the main cilium container image.

Fixes: SUSE/avant-garde#1923

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky vadorovsky merged commit c408d09 into SUSE:maintenance-caasp-v4.2 Dec 11, 2020
@vadorovsky vadorovsky deleted the 4.2-fix-image-list branch December 11, 2020 12:45
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.

4 participants