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

chore: add unmarshal functions for task config policies #9896

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

kkunapuli
Copy link
Contributor

@kkunapuli kkunapuli commented Sep 5, 2024

Ticket

CM-502

Description

Introduce unmarshal functions to process user input for task config policies.

Test Plan

Covered by unit tests.

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Sep 5, 2024
Copy link

netlify bot commented Sep 5, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit a0e8a1a
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66db1d5614dbfc000812d527

func MarshalConfigPolicy(configPolicy interface{}) *structpb.Struct {
return protoutils.ToStruct(configPolicy)

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the symmetry of having a function defined for discoverability, but it's not adding any new functionality.

return false
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the workload type defined in model instead and moving workload type validation to configpolicy.utils.

@@ -11,15 +11,14 @@ message PutWorkspaceConfigPoliciesRequest {
// The workspace the config policies apply to. Use global API for
// global config policies.
int32 workspace_id = 1;
// Specifies which workload type the config policies apply to: EXPERIMENT or
// NTSC.
// The workload type the config policies apply to: EXPERIMENT or NTSC.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed I had missed some spots when fixing descriptions in #9880. Fixing it now.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes missing coverage. Please review.

Project coverage is 54.68%. Comparing base (55b3f9b) to head (a0e8a1a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
master/internal/api_config_policies.go 0.00% 8 Missing ⚠️
master/internal/configpolicy/utils.go 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9896      +/-   ##
==========================================
- Coverage   54.80%   54.68%   -0.13%     
==========================================
  Files        1237     1264      +27     
  Lines      154974   156878    +1904     
  Branches     3597     3597              
==========================================
+ Hits        84938    85790     +852     
- Misses      69905    70957    +1052     
  Partials      131      131              
Flag Coverage Δ
backend 45.22% <71.87%> (-0.02%) ⬇️
harness 72.62% <ø> (ø)
web 54.43% <ø> (ø)

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

Files with missing lines Coverage Δ
master/internal/configpolicy/utils.go 95.83% <95.83%> (ø)
master/internal/api_config_policies.go 0.00% <0.00%> (ø)

... and 29 files with indirect coverage changes

@kkunapuli kkunapuli marked this pull request as ready for review September 5, 2024 19:40
@kkunapuli kkunapuli requested a review from a team as a code owner September 5, 2024 19:40
@kkunapuli kkunapuli requested review from carolinaecalderon and salonig23 and removed request for carolinaecalderon September 5, 2024 19:40
Copy link
Contributor

@salonig23 salonig23 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work and good test coverage!

@kkunapuli kkunapuli merged commit 4a28c10 into main Sep 6, 2024
82 of 96 checks passed
@kkunapuli kkunapuli deleted the kunapuli/marshal-functions branch September 6, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants