Address issue 39622

[drawsmcgraw]

Worked with @rkgrunt to get this one figured out.

The docs say to branch off of the oldest branch with the bug in question, which is why I'm requesting a PR into the 2015.5 branch. Apologies if I've misunderstood the protocol.

What does this PR do?

Fix boto_vpc.create_subnet() to correctly apply user-defined tags for subnets.

What issues does this PR fix or reference?

issue #39622

Previous Behavior

boto_vpc.create_subnet() fails if the user specifies tags.

New Behavior

Tags are correctly applied to new subnets.

Tests written?

No, but has been tested in our dev environment against AWS.

Please review Salt's Contributing Guide for best practices.

[ghost]

@drawsmcgraw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @thedrow, @rallytime and @colinbjohnson to be potential reviewers.

[drawsmcgraw]

Woops! Just pushed a new commit fixing the lint errors.

[cachedout]

@drawsmcgraw This automated test failed:

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/8863/testReport/junit/unit.modules.boto_vpc_test/BotoVpcTestCase/test_that_when_describing_vpc_by_id_it_returns_the_dict_of_properties_returns_true/

Could you take another look, please? Thanks!

[thedrow]

I'm not sure why the test fails. I wrote those a long time ago.
This PR looks good otherwise. I'd of course rather have more coverage to ensure this won't regress.

[drawsmcgraw]

@thedrow I'm also confused on why that test fails as it's querying details of a VPC and the PR in question involves tags in subnets.

What's the "proper" answer in this situation? Is it possible that the test needs attention/updating? I'm about to test my PR again just to be certain but am open to input.

[cachedout]

I totally missed what you wrote above. This actually needs to be submitted to the 2016.3 branch. Our policy is that it should be on the oldest supported branch, which is currently 2016.3. We no longer maintain 2015.5. Let's see if that clears up the failing test along the way. It might. :]

[drawsmcgraw]

Ah! I'll update the PR next time i get a chance. Thanks @cachedout!

…

On Feb 27, 2017 19:22, "Mike Place" ***@***.***> wrote: I totally missed what you wrote above. This actually needs to be submitted to the 2016.3 branch. Our policy is that it should be on the oldest *supported* branch, which is currently 2016.3. We no longer maintain 2015.5. Let's see if that clears up the failing test along the way. It might. :] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39624 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEecH-EY99ukklJpaV08dlNkZonDa9c8ks5rg2jEgaJpZM4MKoDw> .

[drawsmcgraw]

@cachedout, I've edited this PR to request a merge into 2016.3. I'm not sure if something needs to be done or if I'm just impatient re: waiting for automated tests.

For future cases - what's the best way to tell what the oldest supported branch of Salt is?

[cachedout]

@drawsmcgraw We always maintain the two most recent. So whatever the current release branch is and the branch immediately prior.

[cachedout]

Go Go Jenkins!

[cachedout]

I tested this locally and after rebasing 2016.3 onto it, the tests passed. I think this is good to merge.

[drawsmcgraw]

Thanks @cachedout! You guys are the best!

[cachedout]

Too kind! Thanks for all your help, @drawsmcgraw. We're always very grateful. :]