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 type ResourcePoolName string #8978

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented Mar 8, 2024

Description

Introduce a new type ResourcePoolName string, that replaces resource pool name string input/output in RM methods & provides more description where used.

Test Plan

Pass existing tests

Commentary (optional)

I couldn't substitute ResourcePoolName type into sproto messages because of an import cycle -- ideally, as we move away from using sproto messages and simply passing in the variables, we can use ResourcePoolName wherever we pass in a string resource pool name.

Checklist

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

Ticket

DET-76

@cla-bot cla-bot bot added the cla-signed label Mar 8, 2024
Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 0ee20b0
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65ef29dce59403000848c3ab
😎 Deploy Preview https://deploy-preview-8978--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 47.49%. Comparing base (6e1acf4) to head (0ee20b0).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8978   +/-   ##
=======================================
  Coverage   47.49%   47.49%           
=======================================
  Files        1167     1168    +1     
  Lines      176280   176284    +4     
  Branches     2352     2352           
=======================================
+ Hits        83720    83724    +4     
  Misses      92402    92402           
  Partials      158      158           
Flag Coverage Δ
backend 42.57% <33.33%> (+<0.01%) ⬆️
harness 64.01% <ø> (ø)
web 42.83% <ø> (ø)

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

Files Coverage Δ
master/internal/api_command.go 47.08% <100.00%> (ø)
master/internal/api_resourcepool.go 59.11% <100.00%> (ø)
master/internal/checkpoint_gc.go 65.93% <100.00%> (ø)
master/internal/job/jobservice/jobservice.go 8.46% <ø> (ø)
master/internal/rm/resource_manager_iface.go 100.00% <100.00%> (ø)
master/internal/spec_util.go 55.81% <100.00%> (-0.51%) ⬇️
master/internal/sproto/jobs.go 17.39% <ø> (ø)
master/internal/api_experiment.go 54.84% <0.00%> (ø)
master/internal/api_generic_tasks.go 8.95% <0.00%> (-0.02%) ⬇️
master/internal/api_job.go 0.00% <0.00%> (ø)
... and 4 more

... and 5 files with indirect coverage changes

@@ -433,7 +432,7 @@ func (k ResourceManager) ResolveResourcePool(
}
found := false
for _, poolName := range poolNames {
if name == poolName {
if string(name) == poolName {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about defining a String() method for the ResourcePoolName type?

Copy link
Contributor

@kkunapuli kkunapuli Mar 8, 2024

Choose a reason for hiding this comment

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

I'm thinking that it would help differentiate the cases where we're creating a new ResourcePoolName using string( < text > ) and accessing the value of ResourcePoolName using resourcePoolName.String().

Copy link
Contributor

Choose a reason for hiding this comment

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

i like this. i do it in most cases. i know string(name) works just fine, but if you do type ResourcePoolName string and define String() on it, then refactoring ResourcePoolName to not be a string is easier.

but, i mean, it's easy either way, so that isn't a killer argument.

type GetJobQStats struct {
ResourcePool string
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking this change wasn't included by accident? It's fine to include but seems slightly unrelated to the rest of the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I'll add it in the commentary, but this is dead code -- I just came across it in this PR, but we should remove it anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to include it in the commentary! It's fine as is.

Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

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

LGTM! Couple minor questions but nothing that requires changes.

@stoksc
Copy link
Contributor

stoksc commented Mar 11, 2024

don't wait on my review for this, @kkunapuli 's review is sufficient.

@carolinaecalderon carolinaecalderon changed the title chore: replace type ResourcePoolName string chore: add type ResourcePoolName string Mar 11, 2024
@carolinaecalderon carolinaecalderon merged commit 18154f6 into main Mar 11, 2024
78 of 83 checks passed
@carolinaecalderon carolinaecalderon deleted the carolinac/type-rp-string branch March 11, 2024 16:17
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.

3 participants