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

Fixed IP Blocks IT #244

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kdsalvy
Copy link
Contributor

@kdsalvy kdsalvy commented Mar 25, 2022

No description provided.

@waynew waynew self-assigned this Apr 6, 2022
@waynew
Copy link
Contributor

waynew commented Apr 6, 2022

Still seeing

E       AssertionError: assert 'error' not in {'error': 'Error occurred while calling NSX-T API https://nsxt-hostname/ip/api/v1/pools/ip-blocks/. Please check logs for more details.'}

The other test failures should be sorted, so you should be able to merge the latest from the main branch into your PR (or rebase on main).

@waynew
Copy link
Contributor

waynew commented Apr 6, 2022

I think you should be able to see the project on the right-hand column - once you get this test passing, feel free to move this back to the backlog column 👍

@kdsalvy
Copy link
Contributor Author

kdsalvy commented Apr 7, 2022

The test is passing for me, can you please use the config shared?

@waynew
Copy link
Contributor

waynew commented Apr 12, 2022

Note: When testing locally for a code review, ensure that your local repository does not contain any changes. If you're testing with local changes then you'll be testing something different than what others are testing with.

This file seems to be the problem: https://github.com/saltstack/salt-ext-modules-vmware/blob/main/tests/integration/nsxt_config.json

The way the config is loaded also is not following the more typical pattern.

Of course, as we've discussed in the past, the scraper approach is less desirable. If it's possible to gather the required values from the environment it's acceptable to write something like this:

@pytest.fixture
def nsxt_config():
    org_id = os.environ.get("VMWARE_NSXT_ORG_ID")   # assuming NSXT is different than NSX
    ...
    assert org_id, "To run NSX-T tests, set VMWARE_NSXT_ORG_ID environment variable"
    assert refresh_key, "To run NSX-T tests, set VMWARE_REFRESH_KEY environment variable"  # unless the refresh key would be different
    username = do_something(org_id, refresh_key, ...)
    ...
    yield {"username": username, "org_id": org_id, ...}

Obviously these values should be documented, and as much as possible helpers should be provided so that someone who has access to NSX-T should be able to get the required information without outside help.

@kdsalvy
Copy link
Contributor Author

kdsalvy commented Apr 12, 2022

Hi @waynew we can modify the way config is loaded. However, we will still need to replace the config file ( https://github.com/saltstack/salt-ext-modules-vmware/blob/main/tests/integration/nsxt_config.json) with actual values. I'm not sure how to fetch something from the environment if the env related values are not provided in the config.

I will add the instructions to readme file, to add the top level server and credentials. HTH

@waynew
Copy link
Contributor

waynew commented Jul 11, 2022

@kdsalvy are you going to make the changes to this PR or should we go ahead and close it?

@kdsalvy
Copy link
Contributor Author

kdsalvy commented Jul 12, 2022

Hi @waynew , do we need any change in the IT? I have verified it is working fine, on adding the configuration. Any other changes can be taken up as separate PR.

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.

3 participants