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

Project settings #480

Merged
merged 26 commits into from
Oct 3, 2022
Merged

Project settings #480

merged 26 commits into from
Oct 3, 2022

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Sep 27, 2022

TL;DR

Add the project level as a matchable resource. This implements the change IDL pr. Project-level settings will sit at a lower level of specificity than project-domain. The idea is that the front end will issue two queries, one for the project domain level (which it does already today) and one for the project level. The front end will be responsible for merging the results of these two calls.

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

  1. Basically use an empty string Domain to signify a project level setting. This potentially will interfere with the corner case that we might want to one day support of an empty string domain id but I think that is okay for now. Plus this is the same pattern we use for the other columns.
  2. Update the resource repo Get() call to take into account an empty string for the domain.
  3. Add an explicit function to the resource repo that only queries for the project level.
  4. The response from the project level query will also be merged with Admin system level defaults. This is the part that is unique and will need to be removed when the larger settings project is done.
  5. In order to use the execution manager merge function, had to move it to a different package. Interface went to iface.go and the merge function went into util/shared.go
  6. Added the merge call for project level settings into getExecutionConfig

Other changes:

  • Update ResourceInterface for the resource manager to support the three new APIs.
  • Update the ResourceRepoInterface to support a call that is scoped to the project level GetProjectLevel
  • Remove the unused ResourcePriorityDomainLevel and add a project one. Use a higher number in case we one day do want to use an even lower one.
  • Make the MergeUpdateProjectDomainAttributes function more general - use admin.MatchingAttributes instead of admin.ProjectDomainAttributes.
  • Add implementation of new endpoints in adminservice.

Tracking Issue

This is a patch fix for
flyteorg/flyte#2322

It's okay to keep the IDL changes since those make sense more broadly, but the code to merge in system level defaults will need to be removed after the broader settings project is complete.

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #480 (6f0ede5) into master (6cc951f) will increase coverage by 0.02%.
The diff coverage is 65.02%.

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   61.62%   61.64%   +0.02%     
==========================================
  Files         158      158              
  Lines       11293    11484     +191     
==========================================
+ Hits         6959     7079     +120     
- Misses       3615     3671      +56     
- Partials      719      734      +15     
Flag Coverage Δ
unittests 60.58% <64.51%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...kg/manager/impl/validation/attributes_validator.go 83.07% <0.00%> (-8.45%) ⬇️
pkg/rpc/adminservice/attributes.go 0.00% <0.00%> (ø)
pkg/manager/impl/util/shared.go 66.08% <57.14%> (-1.26%) ⬇️
pkg/repositories/gormimpl/resource_repo.go 68.25% <78.26%> (+2.21%) ⬆️
pkg/manager/impl/resources/resource_manager.go 70.03% <81.44%> (+6.45%) ⬆️
pkg/manager/impl/execution_manager.go 73.99% <81.81%> (-0.50%) ⬇️
pkg/repositories/transformers/resource.go 71.53% <83.33%> (+0.66%) ⬆️
pkg/manager/impl/validation/project_validator.go 100.00% <100.00%> (ø)
...implementations/workflow_execution_event_writer.go 80.00% <0.00%> (+40.00%) ⬆️

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

pmahindrakar-oss and others added 5 commits September 30, 2022 23:01
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor merged commit 2f2be8a into master Oct 3, 2022
@wild-endeavor wild-endeavor deleted the project-settings branch October 3, 2022 16:35
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: pmahindrakar-oss <[email protected]>
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