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

Improved onboarding process and tools to set up hostnames #208

Merged
merged 12 commits into from Dec 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2020

This PR adds a new salt config called "onboarding.sls" that manages the initial steps we run on new servers.
These currently being: patching, setting up and configuring the server hostname and finally running the "core" install.

This was a bit delayed because I was hoping to use the salt target to get the hostname.
The PR instead requires a extra bit of pillar data:

salt-ssh TARGET state.apply onboarding pillar='{"host_id": "ocpXX"}'

I wanted to use the target because the above feels it is duplicate information - the "host_id" is in the target name, I just can't query the target name in salt.
I was wondering if there was a work around via Python but I didn't get anywhere when I looked into it.

@ghost ghost requested a review from jpmckinney December 2, 2020 17:51
@ghost ghost self-assigned this Dec 2, 2020
@ghost
Copy link
Author

ghost commented Dec 2, 2020

This PR helps address issue #172

@jpmckinney
Copy link
Member

jpmckinney commented Dec 3, 2020

I think the target name is stored in grains['id']. You can get a list of grains with ./run.py TARGET grains.items.

Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

Looks good! Some comments and suggestions.

docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
docs/deploy/create_server.rst Show resolved Hide resolved
docs/deploy/create_server.rst Outdated Show resolved Hide resolved
Comment on lines 26 to 38

include:
- core.apt
- core.customization
- core.fail2ban
- core.firewall
- core.locale
- core.mail
- core.motd
- core.ntp
- core.sshd
- core.swap
- core.systemd
Copy link
Member

Choose a reason for hiding this comment

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

These aren't dependencies of the init.sls file. I think it's better to leave them in the top file.

Copy link
Author

Choose a reason for hiding this comment

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

Very true, I was considering them dependencies of the "core" install as I don't see us running init.sls without the other files.

The benefit of moving these here is that when onboarding.sls runs include: core this then picks up on all of the other state files. Rather than duplicating them in onboarding.sls.
I see your other comment regarding salt-ssh TARGET 'onboarding,core*' which also mitigates this.
Happy with that approach instead.

Comment on lines +41 to +43
include:
- core

Copy link
Member

Choose a reason for hiding this comment

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

Instead, we can do salt-ssh TARGET state.apply 'onboarding,core*' above.

Copy link
Member

Choose a reason for hiding this comment

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

I'll fix this last bit after merging.

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Applying suggested changes

Rob Hooper and others added 3 commits December 3, 2020 13:41
Co-authored-by: James McKinney <26463+jpmckinney@users.noreply.github.com>
Co-authored-by: James McKinney <26463+jpmckinney@users.noreply.github.com>
@ghost
Copy link
Author

ghost commented Dec 3, 2020

I think the target name is stored in grains['id']. You can get a list of grains with ./run.py TARGET grains.items.

In my testing grains['id'] returns for me the hostname that is set on the server that we want to overwrite.

@jpmckinney
Copy link
Member

In my testing grains['id'] returns for me the hostname that is set on the server that we want to overwrite.

Aha, it must have only worked because the server I was working on had the hostname equal to the target name :)

Co-authored-by: James McKinney <26463+jpmckinney@users.noreply.github.com>
@jpmckinney
Copy link
Member

@RobHooper Can you fix the conflicts, then let me know when it's ready for review?

@jpmckinney jpmckinney merged commit 6101eb3 into master Dec 8, 2020
@jpmckinney jpmckinney deleted the hostname-format-changes branch December 8, 2020 01:11
jpmckinney added a commit that referenced this pull request Dec 8, 2020
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.

None yet

1 participant