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

Supports all valid protos for remote sources #53462

Merged
merged 3 commits into from Dec 28, 2019

Conversation

lorengordon
Copy link
Contributor

@lorengordon lorengordon commented Jun 12, 2019

What does this PR do?

Supports all valid protos for remote file sources

What issues does this PR fix or reference?

Fixes #13971

Previous Behavior

Previously, could not use an s3:// uri in pkg.installed in sources nor as the installer for winrepo.

New Behavior

Now all valid remote file sources work, including s3://.

Tests written?

Maybe?

Commits signed with GPG?

Yes

@twangboy
Copy link
Contributor

@twangboy twangboy commented Jun 14, 2019

@lorengordon Looks like some of the unit test failures are related to this change

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jun 14, 2019

@twangboy can you point me at which ones? It looked to me like the tests were all messed up, and I saw a bunch of work in other PRs trying to clean them up. Wasn't sure what was working and what not, so don't really know where to look right now.

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jun 14, 2019

Ok, so the failing win_pkg test cases certainly look related, but why on earth isn't the config module loading?

Traceback (most recent call last):
  File "c:\users\admini~1\appdata\local\temp\kitchen\testing\tests\unit\modules\test_win_pkg.py", line 219, in test_pkg_install_multiple_pkgs
    ret = win_pkg.install(pkgs=['firebox', 'got'], extra_install_flags='-e True -test_flag True')
  File "C:\\Users\\ADMINI~1\\AppData\\Local\\Temp\\kitchen\\testing\salt\modules\win_pkg.py", line 1261, in install
    if __salt__['config.valid_fileproto'](installer):
KeyError: u'config.valid_fileproto'

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jun 14, 2019

@twangboy I am not able to reproduce the failures when I run my changes locally. Any idea what is going on here?

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jun 24, 2019

@twangboy rebased to pull in latest updates and looks like the tests are passing now. no idea what might have been going on before.

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jul 3, 2019

@twangboy any other concerns with this one?

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jul 24, 2019

Well, it appears the windows tests are failing again, or I misread them the last time I looked. But I just don't understand how it is possible for the config module to not be loaded, or to be missing the valid_fileproto function. When I patch a local install of 2018.3.4 with the changes in this branch, it works fine! 🤷‍♂ @saltstack/team-windows any ideas?

Traceback (most recent call last):
  File "c:\users\admini~1\appdata\local\temp\kitchen\testing\tests\unit\modules\test_win_pkg.py", line 219, in test_pkg_install_multiple_pkgs
    ret = win_pkg.install(pkgs=['firebox', 'got'], extra_install_flags='-e True -test_flag True')
  File "C:\\Users\\ADMINI~1\\AppData\\Local\\Temp\\kitchen\\testing\salt\modules\win_pkg.py", line 1261, in install
    if __salt__['config.valid_fileproto'](installer):
KeyError: u'config.valid_fileproto'

@waynew
Copy link
Contributor

@waynew waynew commented Jul 25, 2019

@lorengordon it's possible that it's (not) being mocked in the unit tests

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jul 25, 2019

@waynew Thank you! I think I see it now. The __salt__ object is mocked. 🤦‍♂ Adding the config.valid_fileproto attribute to the mock now!

@lorengordon lorengordon force-pushed the remote-sources branch 2 times, most recently from 650e488 to c0006bf Compare Jul 25, 2019
@twangboy twangboy self-requested a review Jul 26, 2019
@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jul 26, 2019

ok, well now it seems like something is wrong with jenkins? keeps saying the commit cannot be built? 🤷‍♂

@waynew
Copy link
Contributor

@waynew waynew commented Jul 26, 2019

re-run all full

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Jul 26, 2019

@waynew thanks! still a couple odd builds with the "commit cannot be built" msg but the windows builds are now passing! :woot:

@waynew
Copy link
Contributor

@waynew waynew commented Jul 29, 2019

re-run amazon2

@waynew
Copy link
Contributor

@waynew waynew commented Jul 29, 2019

re-run ubuntu-1804

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Oct 4, 2019

I'll try to look through the tests next week to see if the remote protos are exercised somewhere. I feel like I looked originally and found nothing, but that was the 2018.3 branch. I'll look again on master. I'm definitely not up for writing wholly new tests though. Salt's test framework is a total mystery to me, especially with all these random unexplainable failures. If that's the bar, I'd recommend just pushing (or pr'ing) to my branch.

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Oct 7, 2019

@waynew I updated the one test I could find that exercises the valid protos. I tried to look at tests on the file/pkg/cp modules that support remote sources, but the setup is very confusing to me and it wasn't super clear what was being tested, nor does it seem to make it very easy to extend tests for new protos.

On the plus side, the test suite ran to completion!

@lorengordon lorengordon force-pushed the remote-sources branch 2 times, most recently from cf09f01 to 747a6f1 Compare Oct 15, 2019
@lorengordon lorengordon force-pushed the remote-sources branch 4 times, most recently from f90ad83 to 26cc2a6 Compare Oct 21, 2019
@lorengordon lorengordon force-pushed the remote-sources branch 5 times, most recently from 50391fa to 0d9ebb8 Compare Nov 4, 2019
@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Dec 13, 2019

@twangboy @waynew @Ch3LL Anyone have anything else on this one? Would really appreciate some feedback or a merge.

Ch3LL
Ch3LL approved these changes Dec 19, 2019
@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Dec 19, 2019

@lorengordon looks like there is a conflict. can you resolve the conflict?

@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Dec 21, 2019

@Ch3LL rebased to resolve the conflict (was just a new import in salt/modules/config.py), thanks!

@dwoz
Copy link
Contributor

@dwoz dwoz commented Dec 27, 2019

@lorengordon I've restarted the test suite that failed. When it passes we'll merge. Thanks for your hard work an patience.

@dwoz dwoz merged commit bbe6a31 into saltstack:master Dec 28, 2019
49 checks passed
@lorengordon
Copy link
Contributor Author

@lorengordon lorengordon commented Dec 28, 2019

Thanks @dwoz! What's the release strategy now? The docs all still talk about develop and the release branches. Will this make it into 2018.3 or 2019.2, or only some future, not-yet-released version?

@lorengordon lorengordon deleted the remote-sources branch Dec 28, 2019
@waynew
Copy link
Contributor

@waynew waynew commented Dec 30, 2019

@lorengordon this will make it into Sodium. We plan to release an RC soon (next week or two IIRC, but @dwoz can check me on that).

The docs aren't yet up-to-date. I'm working on that today.

@waynew
Copy link
Contributor

@waynew waynew commented Jan 7, 2020

🙃 apparently I mixed up my codenames. It will make it into Sodium, of course, but it will also make it into Neon.

(the rest of the information is sound, though)

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

Successfully merging this pull request may close these issues.

6 participants