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-1369] Shim: Read configuration from configmaps and deprecate legacy configs #480

Closed
wants to merge 1 commit into from

Conversation

craigcondit
Copy link
Contributor

What is this PR for?

Read configuration from new ConfigMap entries in addition to legacy values, and deprecate old configuration sources.

[WIP] Based on and depends on from YUNIKORN-1365.

What type of PR is it?

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

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1369

How should this be tested?

Screenshots (if appropriate)

Questions:

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

@craigcondit craigcondit self-assigned this Nov 14, 2022
@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #480 (f63cb0d) into master (38bf841) will increase coverage by 1.24%.
The diff coverage is 85.94%.

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   68.06%   69.30%   +1.24%     
==========================================
  Files          42       42              
  Lines        7065     7592     +527     
==========================================
+ Hits         4809     5262     +453     
- Misses       2086     2153      +67     
- Partials      170      177       +7     
Impacted Files Coverage Δ
pkg/cache/context.go 39.86% <3.03%> (-2.87%) ⬇️
pkg/shim/scheduler.go 72.85% <80.00%> (+0.12%) ⬆️
pkg/conf/schedulerconf.go 87.80% <93.34%> (+26.80%) ⬆️
pkg/common/utils/utils.go 63.90% <100.00%> (-0.67%) ⬇️
...missioncontrollers/webhook/admission_controller.go 70.78% <100.00%> (ø)
pkg/schedulerplugin/conf/pluginconf.go 92.23% <100.00%> (+5.16%) ⬆️

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

@craigcondit craigcondit force-pushed the YUNIKORN-1369 branch 3 times, most recently from e7e082b to bcee1e1 Compare November 16, 2022 02:40
@craigcondit craigcondit changed the title [YUNIKORN-1369] [WIP] Shim: Read configuration from configmap and deprecate legacy configs [YUNIKORN-1369] Shim: Read configuration from configmaps and deprecate legacy configs Nov 16, 2022
@craigcondit craigcondit marked this pull request as ready for review November 16, 2022 02:41
case cache.DeletedFinalStateUnknown:
configmap = utils.Convert2ConfigMap(obj)
default:
log.Logger().Debug("unable to convert to configmap")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's an unlikely code path but I'd use an Error() or Warn() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I'll update in the next version.

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1 LGTM (pending small change related to logging)

@craigcondit
Copy link
Contributor Author

Merged to master, updated Debug() to Warn() as requested.

@craigcondit craigcondit deleted the YUNIKORN-1369 branch November 16, 2022 14:37
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