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

Adding labels to admiral generated service entries #281

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

vrushalijoshi
Copy link
Collaborator

Update to SE modification flow, in order to add env, prefix and associated GTP details to SE labels.

Copy link
Contributor

@aattuluri aattuluri 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 overall, added comments.
Also some PR checks are failing.

}
if seDr.SeDnsPrefix != "" && seDr.SeDnsPrefix != common.Default {
newServiceEntry.Labels["dnsPrefix"] = seDr.SeDnsPrefix
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this spec for label names, the above might not work. We can have dns-prefix?
Also why labels and not an annotations? Are you using this as filter for querying?

Copy link
Collaborator Author

@vrushalijoshi vrushalijoshi Feb 7, 2023

Choose a reason for hiding this comment

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

Good catch will update the name, having said that when I tested it on a test cluster it did create SE with this label. I don't see use case where we might need to filter based on dns prefix or GTP, will update these to be annotation.
Will keep env as label, since identity is today getting added and label, and may be required to filter endpoints, let me know if this is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool, updated code based on this

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Merging #281 (3d83184) into master (61b8378) will increase coverage by 0.04%.
The diff coverage is 96.55%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   74.18%   74.22%   +0.04%     
==========================================
  Files          32       32              
  Lines        3266     3279      +13     
==========================================
+ Hits         2423     2434      +11     
- Misses        695      697       +2     
  Partials      148      148              
Impacted Files Coverage Δ
admiral/pkg/clusters/handler.go 67.48% <92.85%> (-0.19%) ⬇️
admiral/pkg/clusters/serviceentry.go 82.20% <100.00%> (+0.35%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vjoshi3 added 2 commits February 8, 2023 11:42
Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com>
Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com>
Copy link
Contributor

@aattuluri aattuluri left a comment

Choose a reason for hiding this comment

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

lgtm

@vrushalijoshi vrushalijoshi merged commit 34e2a7b into master Feb 9, 2023
@vrushalijoshi vrushalijoshi deleted the update-se-annotation-labels branch February 9, 2023 23:15
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.

None yet

3 participants