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

Creds management #53

Merged
merged 41 commits into from
Nov 11, 2020
Merged

Creds management #53

merged 41 commits into from
Nov 11, 2020

Conversation

zhenik
Copy link
Contributor

@zhenik zhenik commented Nov 9, 2020

Closes/fixes/resolves issue(s)?

What was added/changed/fixed?

  • [bump] postgres and minio to 0.3.0
  • [refactor] now there are 2 standalone examples
  • [add] variables
  • [update] documentation

todo:

  • documentation

Related issue(s)? [Optional]

Others [Optional]

Checklist (after created PR)

  • Added reviewers
  • Added assignee(s)
  • Added label(s)
  • Added to project
  • Added to milestone

@zhenik zhenik added this to the 0.3.0 milestone Nov 9, 2020
@zhenik zhenik self-assigned this Nov 9, 2020
@zhenik zhenik added this to Review in progress in Team DataStack via automation Nov 9, 2020
@zhenik zhenik marked this pull request as ready for review November 9, 2020 21:19
@zhenik zhenik requested review from claesgill, pdmthorsrud and dangernil and removed request for claesgill and pdmthorsrud November 9, 2020 21:19
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dev/vagrant/conf/nomad/00-enable_vault.hcl Outdated Show resolved Hide resolved
example/README.md Outdated Show resolved Hide resolved
example/standalone-vault-provided-creds/main.tf Outdated Show resolved Hide resolved
main.tf Show resolved Hide resolved
outputs.tf Show resolved Hide resolved
variables.tf Show resolved Hide resolved
variables.tf Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
dev/ansible/20_upload_files.yml Show resolved Hide resolved
dev/vagrant/conf/nomad/00-enable_vault.hcl Show resolved Hide resolved
Comment on lines 22 to 23
access_key = "minio"
secret_key = "minio123"
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a clear separation between box's minio, and the minio module

Suggested change
access_key = "minio"
secret_key = "minio123"
box_minio_access_key = "minio"
box_minio_secret_key = "minio123"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@zhenik zhenik Nov 10, 2020

Choose a reason for hiding this comment

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

It is not necessary, because box's minio creds are hardcoded in hive.hcl, we do not have variables for that ,

artifact {
  source = "s3::http://127.0.0.1:9000/dev/tmp/hive_local.tar"
  options {
    aws_access_key_id     = "minioadmin"
    aws_access_key_secret = "minioadmin"
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, well, just remove it completely then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a way how to test the newly build image, without pulling from the docker registry.

This functionality uses only with box, but we still need it, if we want to test changes in docker image, before releasing docker image and pushing it to the registry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I didn't mean remove the artifact stanza, I meant the variables set in the .tf file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have provided comments about it, by ur request. I hoped it could be better explained that way https://github.com/fredrikhgrelland/terraform-nomad-hive/blob/issue_44_creds_management/example/standalone-vault-provided-creds/main.tf#L15-L25

example/standalone-user-provided-creds/main.tf Outdated Show resolved Hide resolved
main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
claesgill
claesgill previously approved these changes Nov 10, 2020
Copy link
Contributor

@claesgill claesgill left a comment

Choose a reason for hiding this comment

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

LGTM

Team DataStack automation moved this from Review in progress to Reviewer approved Nov 10, 2020
pdmthorsrud
pdmthorsrud previously approved these changes Nov 10, 2020
Copy link
Contributor

@pdmthorsrud pdmthorsrud left a comment

Choose a reason for hiding this comment

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

LGTM

dangernil
dangernil previously approved these changes Nov 10, 2020
Copy link
Contributor

@dangernil dangernil left a comment

Choose a reason for hiding this comment

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

LGTM

zhenik and others added 16 commits November 10, 2020 16:28
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Co-authored-by: Claes Gill <claes@claesgill.com>
Team DataStack automation moved this from Reviewer approved to Review in progress Nov 10, 2020
Copy link
Contributor

@pdmthorsrud pdmthorsrud left a comment

Choose a reason for hiding this comment

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

LGTM

Team DataStack automation moved this from Review in progress to Reviewer approved Nov 11, 2020
@zhenik zhenik merged commit 989d774 into master Nov 11, 2020
Team DataStack automation moved this from Reviewer approved to Pull Requests merged and closed Nov 11, 2020
@zhenik zhenik deleted the issue_44_creds_management branch November 11, 2020 07:03
@fredrikhgrelland fredrikhgrelland removed this from Pull Requests merged and closed in Team DataStack Mar 10, 2021
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.

Bump dependent modules New output variable for port Upgrade to 0.7.x Improve credentials management
4 participants