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 test in npm state. #30257

Merged
merged 1 commit into from Jan 12, 2016

Conversation

Projects
None yet
4 participants
@abednarik
Contributor

abednarik commented Jan 10, 2016

Updated nmp module to append --dry-run when executing nmp state in test mode.

Fixes #30250.

Show outdated Hide outdated salt/states/npm.py Outdated
@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 10, 2016

Contributor

I think this looks good @abednarik. I left one inline comment that needs to be addressed, but once that is fixed up we can get this in.

I was just going to link you to the docs mentioning the test state, but I just realized the changes dictionary isn't mentioned there. You can see a quick example here of what the changes dict should look like. Ideally this entire function would have changes information in each relevant return, but that update can be for another day, if you like.

Contributor

rallytime commented Jan 10, 2016

I think this looks good @abednarik. I left one inline comment that needs to be addressed, but once that is fixed up we can get this in.

I was just going to link you to the docs mentioning the test state, but I just realized the changes dictionary isn't mentioned there. You can see a quick example here of what the changes dict should look like. Ideally this entire function would have changes information in each relevant return, but that update can be for another day, if you like.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 10, 2016

Contributor

I filed #30258 to remind us to add a changes example to the test=true docs. :)

Contributor

rallytime commented Jan 10, 2016

I filed #30258 to remind us to add a changes example to the test=true docs. :)

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 10, 2016

Contributor

Thanks for those examples, i will get that here.

Cheers.

Contributor

abednarik commented Jan 10, 2016

Thanks for those examples, i will get that here.

Cheers.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 10, 2016

Contributor

Hope now is better. I didn't want to leave this as it is and i made this really quickly. I will review it when I get some time, since this one is not tested at all.

Contributor

abednarik commented Jan 10, 2016

Hope now is better. I didn't want to leave this as it is and i made this really quickly. I will review it when I get some time, since this one is not tested at all.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 11, 2016

Contributor

@abednarik Thanks again for looking at that! I'm sorry to be nit-picky here, but can you make sure that the changes dictionary contains an old and a new key? I'm not sure if there will be any "old" data to populate here (I'm not very familiar with this particular function), but we want to make sure those two key levels are consistent for users. You can see an example of how that works with the changes dict in the npm.installed state.

Contributor

rallytime commented Jan 11, 2016

@abednarik Thanks again for looking at that! I'm sorry to be nit-picky here, but can you make sure that the changes dictionary contains an old and a new key? I'm not sure if there will be any "old" data to populate here (I'm not very familiar with this particular function), but we want to make sure those two key levels are consistent for users. You can see an example of how that works with the changes dict in the npm.installed state.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 11, 2016

Contributor

Sure thing, and you are welcome to to be nit-picky here, is the way to go.
Also i didn't notice that test were failing. Do you want me to have a look?
Still work to do, for me, regarding test what I should try at least :)

2016-01-11 15:15 GMT-03:00 Nicole Thomas notifications@github.com:

@abednarik https://github.com/abednarik Thanks again for looking at
that! I'm sorry to be nit-picky here, but can you make sure that the
changes dictionary contains an old and a new key? I'm not sure if there
will be any "old" data to populate here (I'm not very familiar with this
particular function), but we want to make sure those two key levels are
consistent for users. You can see an example of how that works with the
changes dict in the npm.installed

ret['changes'] = {'old': [], 'new': pkgs_to_install}

state.


Reply to this email directly or view it on GitHub
#30257 (comment).

Contributor

abednarik commented Jan 11, 2016

Sure thing, and you are welcome to to be nit-picky here, is the way to go.
Also i didn't notice that test were failing. Do you want me to have a look?
Still work to do, for me, regarding test what I should try at least :)

2016-01-11 15:15 GMT-03:00 Nicole Thomas notifications@github.com:

@abednarik https://github.com/abednarik Thanks again for looking at
that! I'm sorry to be nit-picky here, but can you make sure that the
changes dictionary contains an old and a new key? I'm not sure if there
will be any "old" data to populate here (I'm not very familiar with this
particular function), but we want to make sure those two key levels are
consistent for users. You can see an example of how that works with the
changes dict in the npm.installed

ret['changes'] = {'old': [], 'new': pkgs_to_install}

state.


Reply to this email directly or view it on GitHub
#30257 (comment).

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 11, 2016

Contributor

@abednarik Oh, the majority of the tests that are failing are not related to this code. There is only one test that is failing that is related to your change, and it is this one. I suspect that when the tests were written for this state, the writer didn't realize we should be looking for __opts__['test']. If you could take a look, that would be fantastic. If you need any help updating it, just let us know.

Contributor

rallytime commented Jan 11, 2016

@abednarik Oh, the majority of the tests that are failing are not related to this code. There is only one test that is failing that is related to your change, and it is this one. I suspect that when the tests were written for this state, the writer didn't realize we should be looking for __opts__['test']. If you could take a look, that would be fantastic. If you need any help updating it, just let us know.

Add test in npm state.
Updated nmp module to append --dry-run when executing nmp state in test mode.

Fixes #30250.
@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 11, 2016

Contributor

@rallytime Great that test failed. because i found that I was trying to get the result of call when this command fails, which is a mistake :) Also, if nothing change, this should be False, not empty. Anyway, I think this should be better.

Contributor

abednarik commented Jan 11, 2016

@rallytime Great that test failed. because i found that I was trying to get the result of call when this command fails, which is a mistake :) Also, if nothing change, this should be False, not empty. Anyway, I think this should be better.

@rallytime

This comment has been minimized.

Show comment
Hide comment
@rallytime

rallytime Jan 12, 2016

Contributor

@abednarik Thanks for fixing that up. Looks good!

Contributor

rallytime commented Jan 12, 2016

@abednarik Thanks for fixing that up. Looks good!

cachedout added a commit that referenced this pull request Jan 12, 2016

@cachedout cachedout merged commit a9366f9 into saltstack:develop Jan 12, 2016

2 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #11204 — FAILURE
Details
jenkins/salt-pr-rs-ubuntu14.04-n Salt PR - RS Ubuntu 14 #8696 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12615 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12314 — SUCCESS
Details

@abednarik abednarik deleted the abednarik:add_dry_run_in_npm branch Jan 12, 2016

@@ -107,6 +108,11 @@ def install(pkg=None,
.. versionadded::2015.9.0
dry_run
Wether or not to run NPM install with --dry-run flag.

This comment has been minimized.

@mbarrien

mbarrien Jan 12, 2016

Contributor

<spellingnazi>Whether misspelled</spellingnazi>

@mbarrien

mbarrien Jan 12, 2016

Contributor

<spellingnazi>Whether misspelled</spellingnazi>

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 13, 2016

Contributor

Thanks @mbarrien

Contributor

abednarik commented Jan 13, 2016

Thanks @mbarrien

cachedout added a commit that referenced this pull request Jan 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment