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

gloo: copy labels from upstream #932

Merged
merged 3 commits into from
Jun 15, 2021

Conversation

knechtionscoding
Copy link
Contributor

Copy the Labels over from the upstream as well as config values. This ensures that gatekeeper or OPA validations can pass.

Signed-off-by: Hans Knecht <Hans.Knecht@missionlane.com>
pkg/apis/gloo/gloo/v1/types.go Outdated Show resolved Hide resolved
@@ -276,6 +276,7 @@ func (gr *GlooRouter) getGlooUpstreamKubeService(canary *flaggerv1.Canary, svc *
if glooUpstreamWithConfig != nil {
configSpec := glooUpstreamWithConfig.Spec
upstreamSpec = gloov1.UpstreamSpec{
Labels: configSpec.Labels,
Copy link
Contributor

Choose a reason for hiding this comment

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

glooUpstreamWithConfig.Metadata.Labels right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so? This defines from the configSpec what labels to pass to the other upstream objects. glooUpstreamWithConfig.Metadata.Labels might make sense here: https://github.com/KnechtionsCoding/flagger/blob/35c8957a5585f9aa7d4446fd211c0289baa7a6ea/pkg/router/gloo.go#L310 but I think that declaration is already correct.

But I may be misunderstanding the flow here.

pkg/router/gloo.go Outdated Show resolved Hide resolved
Signed-off-by: Hans Knecht <Hans.Knecht@missionlane.com>
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2021

Codecov Report

Merging #932 (35c8957) into main (8137a25) will decrease coverage by 0.03%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
- Coverage   56.92%   56.89%   -0.04%     
==========================================
  Files          70       70              
  Lines        5825     5832       +7     
==========================================
+ Hits         3316     3318       +2     
- Misses       2018     2022       +4     
- Partials      491      492       +1     
Impacted Files Coverage Δ
pkg/router/gloo.go 73.36% <28.57%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8137a25...35c8957. Read the comment docs.

@stefanprodan
Copy link
Member

Flagger copies labels to the generated objects based on the --include-label-prefix arg. Please use the same logic here.

fix: remove copy of labels

Signed-off-by: Hans Knecht <Hans.Knecht@missionlane.com>
@stefanprodan stefanprodan changed the title feat: copy labels from upstream gloo: copy labels from upstream Jun 15, 2021
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @knechtionscoding

@stefanprodan stefanprodan merged commit 17c310d into fluxcd:main Jun 15, 2021
@knechtionscoding knechtionscoding deleted the feat/gloo-label-copy branch June 15, 2021 16:28
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