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

datasource: add Nomad scheduler config data source. #168

Merged
merged 7 commits into from
Nov 6, 2020

Conversation

jrasell
Copy link
Member

@jrasell jrasell commented Nov 2, 2020

There are no tests, because they unfortunately clash with the scheduler config resource, which updates the config values meaning the datasource tests have a race condition output with that. I'll think how to better approach this, but it highlights why this resource should be used carefully.

The dependancy update is unrelated to this change, but is needed as a result of 438a0b8 so is included.

cgbaker
cgbaker previously approved these changes Nov 2, 2020
Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

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

cool.

the standard trick that we've used for testing these is a resource to set, then a dependent datasource should see the new value. e.g.,
https://github.com/hashicorp/terraform-provider-nomad/blob/master/nomad/data_source_acl_policy_test.go#L36-L48

did that not work here?

Co-authored-by: Chris Baker <1675087+cgbaker@users.noreply.github.com>
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.

LGTM.

It's just missing an entry for the side menu in website/nomad.erb (we need a PR checklist...)

@lgfa29 lgfa29 self-requested a review November 6, 2020 19:45
@lgfa29 lgfa29 merged commit 43abf9f into master Nov 6, 2020
@lgfa29 lgfa29 deleted the f-sched_config-datasource branch November 6, 2020 19:54
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