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.

Sign up for GitHub

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

Jump to bottom

1370 epinio names #1422

Merged
merged 14 commits into from
May 9, 2022
Merged

1370 epinio names #1422

merged 14 commits into from
May 9, 2022

Conversation

jimmykarily
Copy link
Contributor

@jimmykarily jimmykarily commented May 4, 2022 •
edited

Fixes #1370

Sibling PR: epinio/helm-charts#187

@jimmykarily jimmykarily mentioned this pull request May 4, 2022
Merged
@jimmykarily jimmykarily self-assigned this May 4, 2022
jimmykarily
jimmykarily commented May 4, 2022
View reviewed changes
internal/names/names.go Outdated Show resolved Hide resolved
@jimmykarily jimmykarily force-pushed the 1370-epinio-names branch 3 times, most recently from a456c43 to 895e6d9 Compare May 6, 2022 06:50
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022 •
edited

Codecov Report

Merging #1422 (5e4ec2f) into main (50918b5) will decrease coverage by 32.36%.
The diff coverage is 38.98%.

@@            Coverage Diff             @@
##             main   #1422       +/-   ##
==========================================
- Coverage   40.87%   8.50%   -32.37%     
==========================================
  Files         157      78       -79     
  Lines       11377    7444     -3933     
==========================================
- Hits         4650     633     -4017     
- Misses       6131    6773      +642     
+ Partials      596      38      -558
Flag Coverage Δ
acceptance-api ?
acceptance-apps ?
acceptance-cli ?
unittests 8.50% <38.98%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/api/v1/application/exec.go 0.00% <0.00%> (-69.74%) ⬇️
internal/api/v1/application/running.go 0.00% <0.00%> (-36.67%) ⬇️
internal/application/application.go 0.00% <0.00%> (-66.95%) ⬇️
internal/application/workload.go 0.00% <0.00%> (-56.55%) ⬇️
internal/names/names.go 76.66% <85.18%> (+7.10%) ⬆️
main.go 0.00% <0.00%> (-100.00%) ⬇️
internal/api/v1/authtoken.go 0.00% <0.00%> (-100.00%) ⬇️
internal/application/secret.go 0.00% <0.00%> (-91.49%) ⬇️
internal/configurations/configurations.go 0.00% <0.00%> (-80.73%) ⬇️
internal/api/v1/application/stage.go 0.00% <0.00%> (-76.26%) ⬇️
... and 113 more

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 50918b5...5e4ec2f. Read the comment docs.

@jimmykarily jimmykarily marked this pull request as ready for review May 6, 2022 07:30
@jimmykarily jimmykarily requested a review from a team as a code owner May 6, 2022 07:30
andreas-kupries
andreas-kupries previously approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@andreas-kupries andreas-kupries left a comment

Choose a reason for hiding this comment

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

Approved with a question open.

internal/application/workload.go Show resolved Hide resolved
@jimmykarily jimmykarily mentioned this pull request May 6, 2022
Closed
Dimitris Karakasilis and others added 10 commits May 9, 2022 09:37
Remove not needed methods and sanitize generated names
65fcfe4 
to be used as valid kubernetes resource names. Sanitization methods
was copied from the quarks project:

https://github.com/cloudfoundry-incubator/quarks-utils/blob/79684d6444cf43a544367223227242ce573e90c2/pkg/names/names.go#L30
Bump helm-charts submodule to simplify naming of resources
4f5a727
Detect app deployment using labels and use safe characters for resour…
a0a83d5 
…ce names
Use the same hashing algorithm for names like in the helm chart
c2f3516
Wrap error to be able to tell where it came from
a2e5994
Add unit tests for names package
863a346
Address PR comment by adding a check for unexpected multiple app depl…
94b26af 
…oyments
Fix tests
a91fd15
@jimmykarily
Change how we lookup deployments in tests
5313217
Lookup deployments correctly in tests
c81f571
Make tests more robust and lookup replica restarts using correct name
5e4ec2f 
also fix tests by ignoring Carriage Return characters. This broke
because the name of the Pod became longer reaching the limit of the
"terminal".
@jimmykarily jimmykarily merged commit 4b3c038 into main May 9, 2022
@jimmykarily jimmykarily deleted the 1370-epinio-names branch May 9, 2022 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreas-kupries andreas-kupries andreas-kupries approved these changes

Assignees

@jimmykarily jimmykarily

Labels
None yet
Projects
Epinio planning
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Epinio names
3 participants
@jimmykarily @codecov-commenter @andreas-kupries