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.

Sign up for GitHub

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

Jump to bottom

lsr_role2collection.py Introducing --new-role #66

Merged
merged 1 commit into from
Apr 29, 2021
Merged

lsr_role2collection.py Introducing --new-role #66

merged 1 commit into from
Apr 29, 2021

Conversation

spetrosi
Copy link
Contributor

mssql and some other roles need to change this:
- name: Run the role with default parameters include_role: name: linux-system-roles.mssql
to this:
- name: Run the role with default parameters include_role: name: microsoft.sql.server
python lsr_role2collection.py --role mssql --new-role server --namespace microsoft --collection sql

@spetrosi spetrosi mentioned this pull request Apr 21, 2021
Closed
@spetrosi spetrosi requested a review from nhosoi April 21, 2021 12:46
@nhosoi
Copy link
Contributor

nhosoi commented Apr 21, 2021

Looks good to me. May I ask you a couple of things?

  1. Could you fix the minor errors from black?
  2. If you could also fix the issue The test_copy_tree_with_replace unittest checks the roles: incorrectly #67 you figured, I'd appreciate it.
  3. Could you add the option to the auto-maintenance README.md?

Thanks!

@spetrosi
Copy link
Contributor Author

Hi Noriko,

Looks good to me. May I ask you a couple of things?

1. Could you fix [the minor errors from black](https://github.com/linux-system-roles/auto-maintenance/pull/66/checks?check_run_id=2400403675)?

Could you please give me a hint why black tests failed? I see an error message saying that the reformatted file has been reformatted, what is the issue with this?

2. If you could also fix the issue #67 you figured, I'd appreciate it.

I'll try.

3. Could you add the option to the auto-maintenance README.md?

Will do.

@nhosoi
Copy link
Contributor

nhosoi commented Apr 22, 2021

Hi Noriko,

Looks good to me. May I ask you a couple of things?

1. Could you fix [the minor errors from black](https://github.com/linux-system-roles/auto-maintenance/pull/66/checks?check_run_id=2400403675)?

Could you please give me a hint why black tests failed? I see an error message saying that the reformatted file has been reformatted, what is the issue with this?

Well, all black cares is how the source code looks... There's no logical error nor actually anything wrong in your code. In this case, adding one argument newrolename made the line length limit exceed. And to fold the line, black recommends to put each argument in a line. We are supposed to do copy & paste. ;)

2. If you could also fix the issue #67 you figured, I'd appreciate it.

I'll try.

Thanks!

3. Could you add the option to the auto-maintenance README.md?

Will do.

Thanks, again!

@richm
Copy link
Contributor

richm commented Apr 22, 2021

And to fold the line, black recommends to put each argument in a line

you can use black to format the code for you.

. .tox/black/bin/activate
black lsr_role2collection.py
deactivate

then tox -e black should report success

@nhosoi nhosoi mentioned this pull request Apr 22, 2021
Merged
@spetrosi spetrosi force-pushed the new-role branch 2 times, most recently from 2e8fdab to 1ab4458 Compare April 26, 2021 11:32
nhosoi
nhosoi previously approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@nhosoi nhosoi left a comment

Choose a reason for hiding this comment

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

lgtm
Great job!
Please squash the 6 commits into one before the merge (Also, please do not forget to reset author with git commit --amend --reset-author if it's not automatically done.) Thanks!

@spetrosi spetrosi force-pushed the new-role branch 2 times, most recently from 7175af6 to efccfed Compare April 29, 2021 06:46
@spetrosi spetrosi requested review from nhosoi and richm April 29, 2021 06:47
@spetrosi
Introducing --new-role
006fee8 
mssql needs to change this

  - name: Run the role with default parameters
    include_role:
    name: linux-system-roles.mssql

to this

  - name: Run the role with default parameters
    include_role:
    name: microsoft.sql.server

python lsr_role2collection.py --role mssql --new-role server \
    --namespace microsoft --collection sql

Fix the existing unit test by providing the new newrole argument

Add a unit test to test copy_tree_with_replace with the newrolename arg

Rename roles within the change_roles function

Fix tests to include a proper check of converting the "roles:" keyword

Add docs for the --new-role argument
@spetrosi spetrosi linked an issue Apr 29, 2021 that may be closed by this pull request
The test_copy_tree_with_replace unittest checks the roles: incorrectly #67
Closed
@spetrosi
Copy link
Contributor Author

I've squashed the commits as per Noriko's request, I indeed needed to reset the author, thank you @nhosoi for the instructions.

nhosoi
nhosoi approved these changes Apr 29, 2021
View reviewed changes
Copy link
Contributor

@nhosoi nhosoi left a comment

Choose a reason for hiding this comment

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

lgtm

richm
richm approved these changes Apr 29, 2021
View reviewed changes
Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

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

lgtm

@richm richm merged commit c7f001d into linux-system-roles:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richm richm richm approved these changes

@nhosoi nhosoi nhosoi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The test_copy_tree_with_replace unittest checks the roles: incorrectly
3 participants
@spetrosi @nhosoi @richm