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

Adding new attributes to the quota spec resource #332

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

regner
Copy link
Contributor

@regner regner commented Jun 6, 2023

Fix for #321. Just a first pass adding the attributes.

Still need to add tests and docs.

Just a first pass adding the attributes.
@regner
Copy link
Contributor Author

regner commented Jun 6, 2023

@lgfa29 sorry to poke you directly, but question: with changes like this where an API is changing a lot are there concerns about backwards compatibility? It is all new attributes, nothing renamed or removed. So I assume the actual API calls should work.

@regner
Copy link
Contributor Author

regner commented Jun 6, 2023

Also for reference, here is the go struct for QuotaLimit that the API expects: https://pkg.go.dev/github.com/hashicorp/nomad/api#QuotaLimit

@lgfa29
Copy link
Contributor

lgfa29 commented Jun 7, 2023

It is all new attributes, nothing renamed or removed. So I assume the actual API calls should work.

Yup, adding new attributes should be fine 👍

@regner
Copy link
Contributor Author

regner commented Jun 9, 2023

So problem: I am unable to run the tests for the quota specification as that is an enterprise feature, I don't have an enterprise license, and the install script (https://github.com/hashicorp/terraform-provider-nomad/blob/main/scripts/getnomad.sh#L8) does not install an enterprise version unless you have a license.

Should I just write the tests and hope they pass? Or is there a work around for this?

@regner
Copy link
Contributor Author

regner commented Jun 9, 2023

Question: What is the default value of optional integers?

Example existing code:

"cpu": {
  Type:     schema.TypeInt,
  Optional: true,
},

And the docs for that:

- `cpu` `(int: 0)` - The amount of CPU to limit allocations to. A value of zero
  is treated as unlimited, and a negative value is treated as fully disallowed.

What makes the default 0? Is the provider setting it to 0 somewhere, is that because the Go language treats it as 0, or is that because we know that the Nomad API will default it to 0 if the attribute is not set at all?

Make this slightly more confusing, some of the documentation will list optional strings differently:

From external_volume.html.markdown:

- `capacity_min`: `(string: <optional>)` - Option to signal a minimum volume size. This may not be supported by all storage providers.

From acl_token.html.markdown:

- `name` `(string: "")` - A human-friendly name for this token.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 10, 2023

Hi @regner, sorry for the delay getting back to you here. I had to focus on Nomad 1.6 so I ended up missing some of the other work streams.

Should I just write the tests and hope they pass? Or is there a work around for this?

Yeah, that's not great, but I'm not sure if there's another solution available. For some reason CI is also not running tests on this PR (I will also look into this), so for now feel free to write tests and hope they work. I can run them manually before merge.

What is the default value of optional integers?

You got it right here: "because the Go language treats it as 0".

Make this slightly more confusing, some of the documentation will list optional strings differently:

Normally we mark it fields as <optional> when the zero-value does not impact the resource. In your example, capacity_min zero-value is an empty string "" (again, because of Go) and this value has no special meaning for the resource. For ACL token name, the zero-value may be significant because you will usually see the token name in outputs etc.

Now, that's not an exact science and I'm sure there are plenty of inconsistencies 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants