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

storage/kubernetes: remove shadowed ResourceVersion from Connector #1673

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

ktravis
Copy link
Contributor

@ktravis ktravis commented Mar 17, 2020

Fix #1672

@bonifaido bonifaido self-requested a review March 18, 2020 10:37
@bonifaido
Copy link
Member

Once #1674 gets merged, please rebase on master to have the CI fixed.

@bonifaido
Copy link
Member

I just rebased on master to overcome the MySQL CI issue which is now fixed on master. But the storage tests are failing now for k8s.

@ktravis
Copy link
Contributor Author

ktravis commented Mar 19, 2020

I'm not certain the best way to update these tests, it seems like they rely on a particular ResourceVersion being set, which will now conflict with the one set by the k8s API.

I can remove setting ResourceVersion at all in fromStorageConnector rather than setting it in ObjectMeta (which is probably the right thing to do, given that the k8s API will complain if it is set during a create) - but the tests are comparing the version to an expected value which will not be consistent after the API sets it. If I was writing the test from scratch I would probably set it up to skip the ResourceVersion comparison, if that is acceptable is there a standard way this is being done currently?

Any advice would be appreciated.

@ktravis ktravis force-pushed the fix-k8s-connector-version branch from eb1fd15 to 3533ce7 Compare April 6, 2020 19:58
@ktravis
Copy link
Contributor Author

ktravis commented Apr 6, 2020

In order to make the connector storage tests pass I have ignored the ResourceVersion field from comparison. Testing that this individual field specifically is set does not add any particular extra value to the tests cases, from my perspective. If other behavior is to be tested, it seems reasonable that new/different tests should be written (i.e. old.ResourceVersion != new.ResourceVersion after UpdateConnector calls, etc). Please let me know what else should be done to ensure that this change is ready to merge.

Copy link
Member

@bonifaido bonifaido left a comment

Choose a reason for hiding this comment

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

Thanks for the heads up! I also think that checking those fields is redundant. Ready to merge! 👍

@bonifaido bonifaido merged commit cfae2eb into dexidp:master Apr 7, 2020
@ktravis ktravis deleted the fix-k8s-connector-version branch April 8, 2020 20:11
elffjs pushed a commit to DIMO-Network/dex that referenced this pull request Jun 27, 2022
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.

storage/kubernetes: Connectors are cached indefinitely
2 participants