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

add feature to let unchanged deployments stay deployed #49250

Merged
merged 4 commits into from
Sep 24, 2018

Conversation

maxim-sermin
Copy link
Contributor

What does this PR do?

This PR adds a feature to the state 'jboss7.py' which compares the already deployed artifact with the one that should be deployed and skips the deployment if both artifacts are the same (have an identical hash).

What issues does this PR fix or reference?

None, this is something new.

Previous Behavior

Artifact was always undeployed and deployed again when jboss7.deployed was executed for it. For software which consists of many rather large deployments of which only some have changed, this took some unnecessary amount of time.

New Behavior

When the parameter undeploy_force is set to false and the artifact that should be deployed is the same as the one which is already deployed, the deployment is skipped. When the parameter is set to true or omitted, the previous behaviour is executed.

Tests written?

No

Commits signed with GPG?

No

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

@maxim-sermin
Copy link
Contributor Author

@cachedout done

@cachedout
Copy link
Contributor

@maxim-sermin Can you look into covering these additions with some new tests in salt.tests.unit.states.test_jboss7?

@maxim-sermin
Copy link
Contributor Author

@cachedout Yeah good idea, I just have not done any unit testing in Python yet. So it could be some time before I have the chance to look into this - I will come back to you once I have written some tests.

@cachedout
Copy link
Contributor

@maxim-sermin Sounds good. Feel free to contact us in the community slack channel if you have questions about writing tests.

@cachedout
Copy link
Contributor

@maxim-sermin Just checking in with you here. Anything we can do to help?

@maxim-sermin
Copy link
Contributor Author

@cachedout Much appreciating you taking care of me 😄 Actually I just started to look into Python unit testing yesterday, managed to run some Salt tests - looks good so far. Hope to get this done during the next week, I will get in touch if I come across any issues

@maxim-sermin
Copy link
Contributor Author

@cachedout I was wrong last time, it seems like PyCharm just skipped all the test methods instead of actually running them - no idea why. Do you know a way to run the unit tests locally?

@rallytime
Copy link
Contributor

Hey @maxim-sermin - there are some docs about running salt's test suite here: https://docs.saltstack.com/en/latest/topics/tutorials/writing_tests.html

@maxim-sermin
Copy link
Contributor Author

@cachedout @rallytime Thanks a lot for your support regarding the tests. I am happy to report that I have written some unit tests now. The test methods are mostly copy-paste because that's how I found it to be in test_jboss7.py - if you think it would be better to avoid code repetition, please let me know and I will refactor them.

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

I think this is fine since that was the pattern originally in the testing code. Thank you for writing some new tests!

If you feel inclined, it might be a good idea to refactor some of these tests in the module to be more DRY in another PR, but I think this PR is fine for now.

@rallytime rallytime merged commit edc18b6 into saltstack:develop Sep 24, 2018
garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019
dwoz added a commit that referenced this pull request Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants