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

go api: add resources option to allocations #19079

Closed
wants to merge 1 commit into from

Conversation

robinovitch61
Copy link

@robinovitch61 robinovitch61 commented Nov 13, 2023

There is a documented resources parameter for the http api, but this is impossible to set as "true" right now in the go api as far as I can tell.

I've exposed this option and enhanced the simple test for the other params.

I just noticed that resources is not considered in the go api's list job allocations, even though it returns the same struct of []*AllocationListStub. It would be nice if I could add that too, either in this work or a follow on PR. Let me know your thoughts.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 13, 2023

CLA assistant check
All committers have signed the CLA.

@robinovitch61 robinovitch61 changed the title cli: add resources option to allocations go api: add resources option to allocations Nov 13, 2023
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Hi @robinovitch61 👋

Thanks for catching this! We certainly needs to support this, but I think the QueryOptions may not be the best place to put it since it's a generic struct used in most read requests, and Resources, as far as I can tell, is only used in alloc list and node list (somewhere else to add if you feel like contributing more code 😄).

We also try to keep this struct in sync with its counterpart in the structs package.

Which brings us to the dilemma of Nomad's api package 😬

We would need to modify the signature of Allocations().List() to receive a new parameter for resources, but that would be a breaking change to a public package, which we always try to avoid.

So, I think the best way forward here would be to create a new struct for options spcific to this endpoint and a new method that uses it. Something like this:

type AllocationListOptions struct {
  Resources bool
}
// ...
func (a *Allocations) List(q *QueryOptions) ([]*AllocationListStub, *QueryMeta, error) {
  return a.ListOpts(nil, q)
}

func (a *Allocations) ListOpts(opts *AllocationListOptions, q *QueryOptions) ([]*AllocationListStub, *QueryMeta, error) {
  // Handle opts.Resources and the rest of the list logic
}
// ...

This way Allocations().List() is kept the same and consumers can opt-in to the ListOpts method if they need to. This new method also allows us to add new query params in the future if necessary.

Let us know if you have any questions!

@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
@tgross
Copy link
Member

tgross commented Nov 8, 2024

This has been open a while and left unresolved. I'm going to close it out but if you'd like to revisit we'd be happy to reopen.

@tgross tgross closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
Development

Successfully merging this pull request may close these issues.

4 participants