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

consul*/install.sls refactoring #38

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

Conversation

nledez
Copy link

@nledez nledez commented Nov 4, 2018

consul*/install.sls refactoring.
Come from https://github.com/saltstack-formulas/vault-formula.git

  • use /opt (to remove consul-* versions in autocomplete)
  • checksum check includes GPG one

@nledez
Copy link
Author

nledez commented Nov 4, 2018

Work on Travis issue.

@nledez
Copy link
Author

nledez commented Nov 4, 2018

Ready to merge.

I remove gpg package dependency (remove gpg uninstall package managers too on Ubuntu/Centos).

@aboe76 aboe76 requested a review from jeduardo November 4, 2018 20:14
@jeduardo
Copy link
Contributor

Hey @nledez, nice enhancement here. I've added a couple comments regarding areas where you could use some salt modules instead of calling commands directly and places where you could do a cleanup.

@aboe76
Copy link
Member

aboe76 commented Dec 22, 2018

@jeduardo are you happy with merging this?

@nledez
Copy link
Author

nledez commented Dec 26, 2018

Hi @jeduardo, I'm happy if I can improve this formulas :)
But I didn't see any comments in PR :/

consul_template:
{{ consul_template | yaml }}

import key:
Copy link
Contributor

Choose a reason for hiding this comment

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

@nledez could you try to use the gpg state here instead of calling the command line directly?

consul:
{{ consul | yaml }}

import key:
Copy link
Contributor

Choose a reason for hiding this comment

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

@nledez same comment here regarding importing the GPG using using a salt state instead of running a command directly

/opt/consul/{{ version }}/bin:
archive.extracted:
- source: https://releases.hashicorp.com/consul/{{ version }}/consul_{{ version }}_linux_amd64.zip
- source_hash: /opt/consul/{{ version }}/consul_{{ version }}_SHA256SUMS
Copy link
Contributor

Choose a reason for hiding this comment

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

@nledez I don't think we need to keep the checksums file around in the filesystem after we use them. You could point it to the upstream directly. However I assume you are explicitly downloading them to enforce the GPG check, right?


verify shasums sig:
cmd.run:
- name: gpg --verify /opt/consul-template/{{ version }}/consul-template_{{ version }}_SHA256SUMS.sig /opt/consul-template/{{ version }}/consul-template_{{ version }}_SHA256SUMS
Copy link
Contributor

Choose a reason for hiding this comment

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

@nledez could you try to use the verify function in the salt GPG module instead of calling a command here directly?


verify shasums sig:
cmd.run:
- name: gpg --verify /opt/consul/{{ version }}/consul_{{ version }}_SHA256SUMS.sig /opt/consul/{{ version }}/consul_{{ version }}_SHA256SUMS
Copy link
Contributor

Choose a reason for hiding this comment

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

@nledez same comment here regarding using the Salt module instead of a command.

@jeduardo
Copy link
Contributor

@nledez ah of course, I have forgotten to click on "submit review" after creating the review. You should see comments now I hope.

@jeduardo
Copy link
Contributor

@aboe76 I'd like @nledez to have a look at the suggestions that are now visible before merging this.

@nledez
Copy link
Author

nledez commented Jan 2, 2019

OK I miss GH notifies. I work on it ASAP.

@aboe76
Copy link
Member

aboe76 commented Feb 15, 2019

@nledez do you want to refactor the proposed changes from @jeduardo ?

@aboe76
Copy link
Member

aboe76 commented Mar 16, 2019

@nledez ping

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.

3 participants