-
Notifications
You must be signed in to change notification settings - Fork 273
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
fix linter #344
fix linter #344
Conversation
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
Signed-off-by: gardar <[email protected]>
c209303
to
6e21c3e
Compare
@guenhter, you want to have a look too or we good? |
--- | ||
collections: | ||
- name: ansible.windows | ||
- name: community.docker |
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.
Could this make problems?
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 question, if its mandatory installation it might..
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.
On a second thought, even though if they are mandatory, it should not influence developers much I guess. It just means, that the roles are downloaded I guess but it has no effect on the actual galaxy role. So I'm fine with merging it.
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.
this will enforce the user to install both dependencies.
On one hand this is good, because in ansible 2.16+ the docker_container
role is not found (which is only needed for the docker-executor). On the other hand this is not really needed, when you configure a windows-shell runner (but then ansible.windows
is required for win_command
)
optional dependencies would fit perfectly. But installing dependencies should never hurt, due to the introduced namespaces.
I just have one comment/question. Besides that, this looks amazing. |
Thx for this contribution |
rebased #312 and dropped the two commits changing the name of the role (this was mentioned, to be the blocking point.)