adding boto_elb config parameters to return

[anitakrueger]

What does this PR do?

This PR adds the following parameters to return for get_elb_config. The functionality is needed to be able to determine the hosted zone id to automatically create ALIAS Route53 records for new classic ELBs.

• canonical_hosted_zone_name
• canonical_hosted_zone_name_id
• vpc_id

What issues does this PR fix or reference?

None

Previous Behavior

Cannot get the HostedZoneID that a classic ELB is in even though boto returns the parameter.

New Behavior

boto.get_elb_config returns the canonical_hosted_zone_name_id so that DNS ALIAS records can be created automatically.

Tests written?

No

Commits signed with GPG?

Yes

I understand this will be merged into develop if approved. I'd be grateful if it could be rebased into 2019.2.0 as well.

[ghost]

@anitakrueger, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ryan-lane and @tkwilliams to be potential reviewers.

[dwoz]

@anitakrueger Thanks for your contribution. Please include some unit or integration tests for this change.

[anitakrueger]

Honestly, I don't know how :(
https://github.com/saltstack/salt/blob/develop/tests/unit/modules/test_boto_elb.py has no test function at the moment which tests get_elb_config.
All I'm adding are 3 more parameters to return which are being retrieved with the lb object anyways. Ideally the function would return all of the parameters that boto returns in lb. I looked at the boto3_elb module/state as well, but that seems to solely work for an application load balancer.
I did test this with a classic ELB in AWS.

[anitakrueger]

@dwoz Who can help with tests on this one you think? I honestly don't how to write a whole new test function for adding returning 3 more parameters.

[anitakrueger]

@dwoz is there an update on this please?
I've been following the conversations in Slack about how to always require tests for any PR, however, in the case of 3 lines fixes in execution modules that are not core, that doesn't seem to make sense. In fact, it will prevent the community from contributing.

[waynew]

@anitakrueger for this particular case it looks like it's just a matter of not returning the expected data, so I think a simple test ensuring that it is would be fine.

I would suggest a couple of simple unit tests that mock _get_conn and:

1. mock time.sleep and make sure that if it's throttling errors and all the retries run out, it returns {}
2. mock time.sleep and make sure that non-throttling errors returns {}
3. everything that should be returned is returned (or, at least the subset that you've added here)

There are a number of unit tests that could be a good example, if you need some help hunting them down I can point you in the right direction 👍

[anitakrueger]

@waynew I was in the Salt meeting about testing earlier and don't understand your response. This is adding 3 lines of code that should have been there in the first place and you are asking for a whole new test function. Frankly, I still don't know how to provide it and it will stop me from contributing any further.

What benefit will another test function have for this module?
None, in my opinion.

Yes, it will provide one more code coverage function. But will slow the test suite down.

[waynew]

Hey @anitakrueger thanks for the contribution - I've added the Needs Testcase label - if you're not able to add tests, no problem! We'll work on adding tests to this as soon as we can. In the meantime if there are other community members who are willing & able to put together those tests we could get this merged faster.

[dwoz]

@anitakrueger @waynew

I will look into adding tests for this change sometime in the next week.

[anitakrueger]

Thank you so much @dwoz

I vow, I'll definitely spend time understanding test cases and running unit tests by myself next time I touch anything saltstack based. I can't do atm though :(

[garethgreenaway]

I wrote a quick test to ensure the expected keys are being returned by get_elb_config:

    @mock_ec2_deprecated
    @mock_elb_deprecated
    @skipIf(sys.version_info > (3, 6), 'Disabled for 3.7+ pending https://github.com/spulec/moto/issues/1706.')
    def test_get_elb_config(self):
        '''
        tests that given an valid ids in the form of a list that the boto_elb
        deregister_instances all members of the given list
        '''
        conn_ec2 = boto.ec2.connect_to_region(region, **boto_conn_parameters)
        conn_elb = boto.ec2.elb.connect_to_region(region,
                                                  **boto_conn_parameters)
        zones = [zone.name for zone in conn_ec2.get_all_zones()]
        elb_name = 'TestGetELBConfig'
        load_balancer = conn_elb.create_load_balancer(elb_name, zones,
                                                      [(80, 80, 'http')])
        reservations = conn_ec2.run_instances('ami-08389d60', min_count=3)
        all_instance_ids = [instance.id for instance in reservations.instances]
        load_balancer.register_instances(all_instance_ids)

        # DescribeTags does not appear to be included in moto
        # so mock the _get_all_tags function.  Ideally we wouldn't
        # need to mock this.
        with patch('salt.modules.boto_elb._get_all_tags',
                MagicMock(return_value=None)):
            ret = boto_elb.get_elb_config(elb_name, **conn_parameters)
            _expected_keys = ['subnets',
                              'availability_zones',
                              'canonical_hosted_zone_name_id',
                              'tags',
                              'dns_name',
                              'listeners',
                              'backends',
                              'policies',
                              'vpc_id',
                              'scheme',
                              'canonical_hosted_zone_name',
                              'security_groups']
            for key in _expected_keys:
                self.assertIn(key, ret)

[dhiltonp]

@anitakrueger thanks so much for your patience with this.

Are you up to adding Gareth's test to "tests/unit/states/test_boto_elb.py"?

If not, we can create a new PR with your changes and his test then close this one - but I prefer keeping history all in one place if that works for you!

[sagetherage]

@anitakrueger are you able to rebase this against the master branch?

[sagetherage]

@anitakrueger are you still wanting to merge this and still working on it? can you rebase against master branch?
Reviewed in Grooming this week considering closing

[anitakrueger]

@sagetherage Yes, it would be good to merge this.
I've added the test function.

[anitakrueger]

@sagetherage @waynew Not sure what happened here. I did add the test function. Why was this closed please?

[sagetherage]

@anitakrueger please rebase this against the master branch since we no longer are using develop

[anitakrueger]

I'm not having much luck getting this rebased. I fetched the upstream and checked out master. Then checked out my branch and ran git rebase master. It took ages before it would start the rebase and now it is throwing Patch failed errors left and right. I first thought it was only one, so ran git rebase --skip but there are lots! Something like below for lots and lots of files:

Applying: Update win_pkg.py
Using index info to reconstruct a base tree...
M	salt/modules/win_pkg.py
Falling back to patching base and 3-way merge...
Auto-merging salt/modules/win_pkg.py
CONFLICT (content): Merge conflict in salt/modules/win_pkg.py
error: Failed to merge in the changes.
Patch failed at 0069 Update win_pkg.py
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

No idea what to do honestly.

[waynew]

@anitakrueger Looks like a normal rebase will try to get your changes and everything else.

Doing a git rebase -i salt/master (or upstream, whatever the remote is called) produces a ton of commits. Simply deleting the lines will get those out of the way -- alternatively, assuming a remote name of salt, and assuming yours is anita :

git fetch salt
git checkout salt/master
git cherry-pick 0cd8698364c209def3a4afbfd81410ff44b454ed
git cherry-pick 881d3f48bcb4ebcd1eac492bf3808c73345acb09

Then you should be able to do git push --force anita HEAD:add-boto-elb-config-parameters

If you still have any problems, please let me know!

[waynew]

re-rebased this again, looks like pre-commit is passing now. Hopefully that should get this across the line

tests have been added

[waynew]

Looking better - test failures look unrelated/flaky. Triggered a re-run.

[github-actions]

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.22s
- exit code: 1
The function 'create' on 'salt/modules/boto_elb.py' does not have a 'CLI Example:' in it's docstring

The function 'delete' on 'salt/modules/boto_elb.py' does not have a 'CLI Example:' in it's docstring

The function 'set_attributes' on 'salt/modules/boto_elb.py' does not have a 'CLI Example:' in it's docstring

The function 'set_health_check' on 'salt/modules/boto_elb.py' does not have a 'CLI Example:' in it's docstring

Found 4 errors

---

Thanks again!

[anitakrueger]

Thank you both!