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 local mode and remove deprecated http_instance call #16

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tuxmaster5000
Copy link

This Pull Request (PR) fixes the following issues

@tuxmaster5000 tuxmaster5000 mentioned this pull request Mar 28, 2019
end

def lookup(path, vault_url = nil)
def lookup(path, vault_url = nil, local_token = false, local_token_file = '/etc/vault.token')
Copy link
Contributor

Choose a reason for hiding this comment

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

@tuxmaster5000 have you considered in adding in support for Hashicorp environment variables?

I'm not sure of your use case, but I think that adding support similar to what is already present for VAULT_ADDR might be a better approach than expanding the argument list here.

Copy link
Author

Choose a reason for hiding this comment

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

As far I understood the link, the variable store the token itself. But my idea was to add an option the load the token from an file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested following support for some of the Hashicorp environment variables because it seems like a good idea to follow some of the standard ways Hashicorp supports reading in the token. If we are going to add support for it as a method argument, it might be a good idea to add support reading it from the environment as well? There is already a pattern for reading in the vault_url from the ENV, and I think it would be good to extend that pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this implementation, the code is setting a default location for the token file to be in /etc/vault.token, but the standard place is often ~/.vault-token. Is this the reason for splitting local_token and local_token_file, so we can set that default location? I'm not sure we need to provide that default here, but I'd like to hear why the code is written that way.

@tvpartytonight
Copy link
Contributor

@tuxmaster5000 there was a change commited to master that fixes the failing CI tests; can you rebase your PR onto master so we can have results from CI updated for this? Thanks!

@vox-pupuli-tasks
Copy link

Dear @tuxmaster5000, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @tuxmaster5000, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@tuxmaster5000
Copy link
Author

Because you have change the master to much I can't fix it. So please overtake it.

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.

2 participants