Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Only merge system defaults when querying workflow execution config for project attributes #522

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Feb 15, 2023

TL;DR

The project-level-only matchable attribute endpoint currently merges in defaults from the server config (as this was the quickest way to get things going in light of the need to expose this to the UI and the complexity of the broader settings project). There was a bug in the initial implementation though - these system level defaults should only be merged when the user is querying for workflow execution configs. This just adds a check for that.

See the original PR for more info as well.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Also update some the default config.yaml to work with the new single-binary based demo sandbox.

Tracking Issue

Just noticed this in testing.

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
@wild-endeavor wild-endeavor changed the title don't merge defaults when querying other types Only merge system defaults when querying workflow execution config for project attributes Feb 15, 2023
katrogan
katrogan previously approved these changes Feb 15, 2023
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

Is it worth adding a unit test?

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
katrogan
katrogan previously approved these changes Feb 16, 2023
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #522 (ba99adc) into master (2e156a3) will increase coverage by 1.53%.
The diff coverage is 94.82%.

❗ Current head ba99adc differs from pull request most recent head 543a957. Consider uploading reports for the commit 543a957 to get more accurate results

@@            Coverage Diff             @@
##           master     #522      +/-   ##
==========================================
+ Coverage   60.09%   61.63%   +1.53%     
==========================================
  Files         168      169       +1     
  Lines       15065    12417    -2648     
==========================================
- Hits         9054     7653    -1401     
+ Misses       5211     3964    -1247     
  Partials      800      800              
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
pkg/manager/impl/execution_manager.go 72.94% <90.90%> (+1.56%) ⬆️
pkg/manager/impl/util/resources.go 91.52% <91.52%> (ø)
pkg/manager/impl/node_execution_manager.go 72.54% <100.00%> (+2.61%) ⬆️
pkg/manager/impl/resources/resource_manager.go 70.03% <100.00%> (+5.88%) ⬆️
pkg/manager/impl/task_execution_manager.go 72.27% <100.00%> (+2.98%) ⬆️
pkg/manager/impl/task_manager.go 67.85% <100.00%> (+3.85%) ⬆️
pkg/manager/impl/validation/task_validator.go 87.50% <100.00%> (+0.89%) ⬆️
pkg/repositories/transformers/execution.go 82.60% <100.00%> (+3.24%) ⬆️
pkg/repositories/transformers/node_execution.go 73.42% <100.00%> (+3.76%) ⬆️
pkg/repositories/transformers/task_execution.go 74.82% <100.00%> (+2.99%) ⬆️
... and 154 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@wild-endeavor wild-endeavor merged commit fb003b7 into master Feb 16, 2023
@wild-endeavor wild-endeavor deleted the fix-wec-merge branch February 16, 2023 17:07
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
…r project attributes (#522)

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants