-
Notifications
You must be signed in to change notification settings - Fork 0
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
Automation and optimization work #5
Conversation
inventory/hosts.yml.example
Outdated
ansible_user: <your_user_on_teaparty> | ||
# Set this to your username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to recommend people to configure their username correctly in their SSH config so this file can be used as is?
For example:
Host *.getsol.us
User <username here>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but I think it's out of scope here. I think this should be part of our staff onboarding processes, which should be written down (what people need access to, why, how to get there, etc.). If that exists, I've never seen it (at least, not in the "set up your SSH config correctly" level of detail). If we can get that documentation into shape in an appropriate place, I'd love to remove this hacky inventory creation script nonsense. I'm just not convinced that the correct place is here in this repo.
Feel free to convince me otherwise, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to suggest to document that here, I just don't think that this project should carry the complexity burden of people not having configured their SSH correctly.
Edit: I've created an issue to document this https://github.com/getsolus/solus-team-docs/issues/60
setup_inventory.py
Outdated
@@ -0,0 +1,27 @@ | |||
#!/bin/python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go the ~/.ssh/config
route this should not be needed :S
Co-authored-by: Silke Hofstra <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README needs a step before generating mainstream data about copying hosts.yml.example to the real file and adding the username. Otherwise go-task full-process errors out. Once that's done this PR is good to go.
In my testing, once I set that up and ran go-task full-process, appstream data generation was successful
README.md
Outdated
> Additionally, your GitHub account must have push access to this repository (should be true for all staff). | ||
1. Ensure that your local clone of this repository is set up to be able to push (Cloning via GitHub CLI recommended). | ||
2. Ensure you have `go-task` installed. All interaction with this tooling should be possible through the `Taskfile.yml` in this repository. | ||
3. Run `go-task appstream-init`. This task will install `pyyaml` and `ansible` on your system, and then install the necessary ansible collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a step here to copy and adapt inventory.yml.example
and that the file needs to be named hosts.yml
(due to the rule in .gitignore
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the rule in .gitignore and made the inventory file generic. A username does not need to be listed there if your SSH is configured correctly, per the conversation with @silkeh.
We don't need this to be configured per user if folks set up SSH correctly.
Co-authored-by: Silke Hofstra <[email protected]>
We don't need this to be configured per user if folks set up SSH correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-tested after doing a hard reset on my local copy of the branch so I had the new generic hosts.yml. I already had an entry for teaparty in ~/.ssh.config
.
Verified that appstream data was able to be kicked off an a commit with the data was created as expected.
I Silke's concerns have also been addressed.
Good to go!
Please clean up the git commit message and this can be squashed and merged
This is the (long-awaited) appstream automation PR!
Here's what it does:
Here's what it doesn't include yet, which it needs to before this can be marked ready for review:
A Taskfile with further automation, including step 7 of the current instructions.Updates to README.md documenting the new process.A test plan in the description :)Here's what it doesn't include, and I have no plans to:
Taskfile
in that package's files.Test Plan
Follow the directions in the updated version of the README. That documentation is under test along with the actual process changes.
For reference, the current workflow is under the heading "Regenerating from teaparty server directly" in this repo's README.