Skip to content
This repository was archived by the owner on Sep 16, 2020. It is now read-only.

Don't fail when 'prevent' switch match an asset#511

Merged
john-westcott-iv merged 4 commits intoansible:masterfrom
pilou-:dont_fail_when_prevent_match
Sep 14, 2018
Merged

Don't fail when 'prevent' switch match an asset#511
john-westcott-iv merged 4 commits intoansible:masterfrom
pilou-:dont_fail_when_prevent_match

Conversation

@pilou-
Copy link
Contributor

@pilou- pilou- commented Apr 13, 2018

Import with send command fails when prevent switch match an asset:

$ tower-cli send --prevent user config.json -v
Asset of type user is prevented from importation
Error: One or more errors encountered in provided assets, stopping import

This pull-request allows to avoid this error (import isn't aborted) without importing excluded assets.

@coveralls
Copy link

coveralls commented Apr 13, 2018

Coverage Status

Coverage decreased (-0.02%) to 63.945% when pulling f0da64b on pilou-:dont_fail_when_prevent_match into 4d146cb on ansible:master.

@AlanCoding
Copy link
Member

Looks good to me, @john-westcott-iv does this look reasonable to you?

Copy link

@wwitzel3 wwitzel3 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@john-westcott-iv john-westcott-iv left a comment

Choose a reason for hiding this comment

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

Instead of calling log_ok, please call log_warn. Log ok will produce a green message which indicates no changes required. log_warn will generate a bold magenta warning which should stand out during a manual run. Programmatically we can also check the number of warnings incase we want to take further action if everything in a run wasn't perfect.

@pilou-
Copy link
Contributor Author

pilou- commented Apr 27, 2018

When prevent switch is used, I expect tower-cli to not import this kind of assets. Then when tower-cli reports that some assets aren't imported because that's what I asked, there should be no warning.

@pilou-
Copy link
Contributor Author

pilou- commented May 8, 2018

ping @john-westcott-iv

@john-westcott-iv
Copy link
Member

john-westcott-iv commented May 8, 2018

Apologies for the delay in response, I have been going back and forth on this issue. From a personal usage perspective I would agree. However, when I wrote these functions I assumed a multi user environment where "developers" were handing a "Tower admin" files and the --prevent flag was a security measure. From that perspective, if I was the "Tower admin" I would want it flagged some how (like a warning or an error) that there was something in the file that the user was trying to make that I chose to ignore. That would let me go back to the user and talk with them about why they needed that asset. That is initially why it was a hard error, to force the conversation. What are your thoughts on that use case? One thing was thinking is maybe make it a warning and then we could add another switch like --prevent-type which would let you change it from a warning to either an error or an ok.

@pilou-
Copy link
Contributor Author

pilou- commented May 9, 2018

I propose to keep behavior of --prevent switch unchanged and to add another switch (--exclude or --ignore). This new switch would allow to ignore excluded/ignored kind of assets without raising any warning/error.

@john-westcott-iv
Copy link
Member

I like that idea. If we went with --ignore we could also pass that a value like: --ignore prevent. That would also solve another issue I was thinking about when doing a send/receive which would be maybe I want it to try and migrate the roles but don't fail if a role does not exist on the target. So we could also do --ignore roles. Do you want to try coding up the initial part of the ignore option? If not we can open a ticket and I can take that on.

@pilou-
Copy link
Contributor Author

pilou- commented May 24, 2018

@wwitzel3 @john-westcott-iv : branch has been updated.

This way, if something is listed as both prevent and exclude the prevent will take precedence. A user should have the same asset type in both but prevent has a higher impact over exclude so I would want that one to take precedence.
@pilou-
Copy link
Contributor Author

pilou- commented Jun 3, 2018

@john-westcott-iv your commits look good to me :)

@john-westcott-iv
Copy link
Member

@pilou- I made some minor changes to the code, please have a look at them and let me know if you agree with those.

@pilou-
Copy link
Contributor Author

pilou- commented Jun 3, 2018

@john-westcott-iv that's fine

@pilou-
Copy link
Contributor Author

pilou- commented Jul 24, 2018

@john-westcott-iv @wwitzel3 can we move forward ?

@john-westcott-iv john-westcott-iv merged commit 42c06f2 into ansible:master Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants