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

Roster sshknownhosts #51840

Merged
merged 16 commits into from
May 30, 2019
Merged

Conversation

remijouannet
Copy link
Contributor

@remijouannet remijouannet commented Feb 26, 2019

What does this PR do?

Hello,

I developed a new roster to parse a known_hosts file and generate a roster from it,
i just want to know if that something you would merge, i haven't write the documentation or execute pep8 on the code yet

Tests written?

not yet

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@remijouannet
Copy link
Contributor Author

anyone ?

@waynew
Copy link
Contributor

waynew commented Mar 5, 2019

@remijouannet Thanks for the PR! I'll take a look at this some time today!

@waynew
Copy link
Contributor

waynew commented Mar 5, 2019

Personally I think the addition is a good idea, and seems like it's shaping up well. Obviously some docs, and some automated tests still need to happen, but otherwise seems like a useful addition.

@saltstack/team-core Should salt-ssh be able to build the roster from ~/.ssh/known_hosts? I'm 👍 on the idea. Agree? Disagree?

@waynew waynew added Feature new functionality including changes to functionality and code refactors, etc. Salt-SSH Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged labels Mar 5, 2019
@remijouannet
Copy link
Contributor Author

Hi everyone,
just upping this PR since i havn't got any answer

@waynew
Copy link
Contributor

waynew commented Apr 15, 2019

Well, I guess there aren't any objections :) Couple of failing checks that may or may not be related.

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

If I understand the complaint in Jenkins correctly, the surrounding lines should be the same length as their contents.

Give that a shot and see if it sorts the docs issue!

doc/ref/roster/all/salt.roster.sshknownhosts.rst Outdated Show resolved Hide resolved
doc/ref/roster/all/salt.roster.sshknownhosts.rst Outdated Show resolved Hide resolved
waynew and others added 2 commits April 27, 2019 16:25
Co-Authored-By: remijouannet <remijouannet@gmail.com>
Co-Authored-By: remijouannet <remijouannet@gmail.com>
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Hi @remijouannet this is looking great!

The only thing it currently lacks is some tests. You could write unit or integration tests (or both). I think that tests/unit/ssh/test_roster_defaults.sh might have a reasonable example test you could look at.

Alternatively, there are also tests in tests/integration/ssh/ that you could probably look at 👍

Once you've got some tests, I think this will be totally ready to go!

@remijouannet
Copy link
Contributor Author

Hi, yes really sorry i was really busy at works, I will work on this as soon as i can

@remijouannet
Copy link
Contributor Author

hi @waynew, one month later, i finally found the time to finish this PR, i did some refacto by using the new utils module "roster_matcher", include a unit test and write the documentation
waiting for your review now

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple of doc tweaks and then I'll ✔️ this!

salt/roster/sshknownhosts.py Outdated Show resolved Hide resolved
salt/roster/sshknownhosts.py Outdated Show resolved Hide resolved
salt/roster/sshknownhosts.py Outdated Show resolved Hide resolved
salt/roster/sshknownhosts.py Outdated Show resolved Hide resolved
remijouannet and others added 4 commits May 29, 2019 23:15
Co-Authored-By: Wayne Werner <waynejwerner@gmail.com>
Co-Authored-By: Wayne Werner <waynejwerner@gmail.com>
Co-Authored-By: Wayne Werner <waynejwerner@gmail.com>
@waynew waynew merged commit 97c3da5 into saltstack:develop May 30, 2019
@waynew
Copy link
Contributor

waynew commented May 30, 2019

🎉

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019
garethgreenaway added a commit that referenced this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc. Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged Salt-SSH
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants