Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

tests/integration: fix cleanup steps #94

Closed
wants to merge 1 commit into from
Closed

tests/integration: fix cleanup steps #94

wants to merge 1 commit into from

Conversation

estroz
Copy link

@estroz estroz commented Sep 20, 2019

Both removed steps attempt to delete a quay.io application repository, which will not work with typical account permissions. Deleting all application releases before a test runs should suffice.

/cc @csomh @MartinBasti

@csomh csomh self-requested a review September 20, 2019 09:02
@csomh
Copy link
Contributor

csomh commented Sep 20, 2019

Hey @estroz, thank you for submitting this.

I was trying to find out why it was chosen in the past to delete the whole app repo instead of just deleting all the versions.

A quick git log tests/integration/conftest.py revealed 062dcfa, which is just about doing the opposite of this change: giving up on cleaning up all the versions, and deleting the whole app repo instead.

Now, at this point, I wished I would have given a more detailed explanation in the commit message on why I did this change 😬

But then I looked up the PR (#40) which introduced the commit above. It seems that the change was done to test OMPS's functionality to make some app repos public. That test relied on the fact that when the first version of an operator is pushed and the repo is created, it is created as private.

I will still need to look up, why the above approach was chosen, instead of using some API end-point to change the visibility of the repo. Stay tuned.

@csomh
Copy link
Contributor

csomh commented Sep 20, 2019

It seems there is an API endpoint to change the visibility of repos, but this is also Quay API, so it will require OAuth.

The app-registry endpoint (https://quay.io/cnr/api/v1) doesn't seem to have such a thing.

I would argue to drop this change: deleting the repositories during teardown is the safest thing to do, imho.

@estroz
Copy link
Author

estroz commented Sep 20, 2019

@csomh sounds good! Thanks for the thorough research.

@estroz estroz closed this Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants