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

Add Data Source for a Nomad ACL Policy. #89

Merged
merged 6 commits into from
Oct 30, 2019

Conversation

ddreier
Copy link
Contributor

@ddreier ddreier commented Oct 20, 2019

For #82.

Needed for the Test to work:

  • Instance of Nomad with ACL enabled
  • Run the nomad acl bootstrap command to get a secret_id
  • Set the NOMAD_TOKEN environment variable to the secret_id

Given that there are already tests for the nomad_acl_policy resource that actually create and delete policies, I don't think that this should be a problem.

Needed for the Test to work:
 * Instance of Nomad with ACL enabled
 * Run the `nomad acl bootstrap` command to get a secret_id
 * Set the NOMAD_TOKEN environment variable to the secret_id
@ghost ghost added the size/M label Oct 20, 2019
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.

Thanks for the PR!

Setting the token to run the tests is fine, we have instructions in the README for this, so it's not a problem.

I've added a few change requests, feel free to comment on them if you have any question.

Could you also add a documentation page for this data source? The docs are located under the website folder.

@ddreier
Copy link
Contributor Author

ddreier commented Oct 26, 2019

Thank you for the feedback! I'm working on getting it all implemented now.

@ghost ghost added size/L documentation and removed size/M labels Oct 26, 2019
@ddreier
Copy link
Contributor Author

ddreier commented Oct 26, 2019

I believe that I have everything addressed. Let me know if there's anything else. Thanks!

@lgfa29
Copy link
Contributor

lgfa29 commented Oct 28, 2019

Thanks for the work @ddreier, just a couple of things left that I noticed:

  • add a menu item for the new data source
  • the file name and test function name don't need to have nomad in them

@lgfa29
Copy link
Contributor

lgfa29 commented Oct 28, 2019

I also just merged a PR that might conflict with your work, but it should be simple to fix it. Let me know if you need any help on that.

@lgfa29 lgfa29 requested a review from cgbaker October 28, 2019 17:01
@ddreier
Copy link
Contributor Author

ddreier commented Oct 30, 2019

@lgfa29 thanks! I have pushed those changes.

@lgfa29 lgfa29 self-requested a review October 30, 2019 17:05
@lgfa29
Copy link
Contributor

lgfa29 commented Oct 30, 2019

Thank you for the updates 👍

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.

LGTM. Thanks, @ddreier 🚀

@lgfa29 lgfa29 merged commit dc7c4f7 into hashicorp:master Oct 30, 2019
@ddreier ddreier deleted the datasource_nomad_acl_policy branch October 30, 2019 19:03
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