Skip to content

Conversation

@dsbibby
Copy link

@dsbibby dsbibby commented Nov 18, 2021

Cookie based authentication is deprecated - see VMware KB 78315

This PR changes to use header based authentication instead an in doing so addresses #83 and fixes #87

cookie based auth is deprecated - KB78315
Copy link
Contributor

@bishopbm1 bishopbm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

Will you please update and add some tests to https://github.com/StackStorm-Exchange/stackstorm-vsphere/blob/master/tests/test_action_vmwarelib_tagging.py for the changes.

Also please add a section to the Change log and update the version in the pack.yaml

Thank you!

@bishopbm1
Copy link
Contributor

We had another company confirm and test for vsphere 7 and we testing on vsphere 6 and everything is working as expected. Ready to merge as soon as unit tests and other items are added.

@dsbibby
Copy link
Author

dsbibby commented Mar 19, 2022

Hi, I've been having a look at the existing tests trying to get my head around them. Any pointers/tips greatly received!

The only thought I had so far about this is that unless I'm missing something, it looks like there is no test for the login method at present. As such should the unit tests really be raised under a separate PR to validate the current code behavior. Then this PR could be validated using those tests?

Also, could consider using vcsim as part of the unit testing? I'm not sure if it would fit in to the current test framework or not.

1 similar comment
@dsbibby
Copy link
Author

dsbibby commented Mar 19, 2022

Hi, I've been having a look at the existing tests trying to get my head around them. Any pointers/tips greatly received!

The only thought I had so far about this is that unless I'm missing something, it looks like there is no test for the login method at present. As such should the unit tests really be raised under a separate PR to validate the current code behavior. Then this PR could be validated using those tests?

Also, could consider using vcsim as part of the unit testing? I'm not sure if it would fit in to the current test framework or not.

@bishopbm1
Copy link
Contributor

@dsbibby I added a test for the login method. You are correct that there were not tests on it originally. I had to make an update to fix the CircleCi workflow but we have switched this pack to GitHub Actions. I will be switching that back and updating the ChangeLog and Pack yaml so that we can get this merged.

Please review the changes that I made to the PR and let me know if you have any questions about the changes that I made or would like me to explain anything further. I am in the StackStorm Slack channel also if that is easier!

Copy link
Contributor

@bishopbm1 bishopbm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bishopbm1 bishopbm1 merged commit cabebdb into StackStorm-Exchange:master Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

requests.cookies.CookieConflictError: There are multiple cookies with name, 'vmware-api-session-id'

3 participants