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

[YUNIKORN-1273] Add configurable option to have unique application ids in a namespace #594

Closed
wants to merge 1 commit into from

Conversation

mitdesai
Copy link
Contributor

What is this PR for?

Ability to configure autogenerated application ids. Currently, if the app id is not specified, yunikorn generates the application id with the name 'yunikorn-<name_space>-autogen' and all the pods with no application id are bundled under that name. This change adds an option to have unique auto-generated names. Appends pod uid after the namespace name to make it unique.

First time? Check out the contributing guide - http://yunikorn.apache.org/community/how_to_contribute
Successfully ran these commands in k8shim
make license-check
make lint
make test

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

  • [NO] - The licenses files need update.
  • [NO] - There is breaking changes for older versions.
  • [NO] - It needs documentation.

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

Linter issues on comments, and two other minor comment nits

pkg/admission/util.go Outdated Show resolved Hide resolved
pkg/admission/util_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #594 (df70048) into master (2d91d01) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
+ Coverage   69.95%   70.18%   +0.23%     
==========================================
  Files          47       47              
  Lines        7938     7950      +12     
==========================================
+ Hits         5553     5580      +27     
+ Misses       2177     2164      -13     
+ Partials      208      206       -2     
Impacted Files Coverage Δ
pkg/admission/admission_controller.go 65.93% <100.00%> (-0.27%) ⬇️
pkg/admission/conf/am_conf.go 72.10% <100.00%> (+6.31%) ⬆️
pkg/admission/util.go 95.83% <100.00%> (+1.23%) ⬆️

... and 1 file with indirect coverage changes

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

@mitdesai
Copy link
Contributor Author

Codecov Report

Merging #594 (0f9aed1) into master (2d91d01) will decrease coverage by 0.01%.
The diff coverage is 80.95%.

❗ Current head 0f9aed1 differs from pull request most recent head df70048. Consider uploading reports for the commit df70048 to get more accurate results

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage   69.95%   69.94%   -0.01%     
==========================================
  Files          47       47              
  Lines        7938     7950      +12     
==========================================
+ Hits         5553     5561       +8     
- Misses       2177     2181       +4     
  Partials      208      208              

Impacted Files Coverage Δ
pkg/admission/conf/am_conf.go 64.80% <20.00%> (-0.99%) ⬇️
pkg/admission/admission_controller.go 65.93% <100.00%> (-0.27%) ⬇️
pkg/admission/util.go 95.83% <100.00%> (+1.23%) ⬆️
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

  1. Added tests for am_conf.go
  2. admission_controller.go had a function moved to utils.go hence the difference in the covered lines.

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM

@wilfred-s wilfred-s closed this in 95157f9 May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants