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

Test against multiple versions of operator-courier. #67

Merged
merged 2 commits into from Apr 29, 2019

Conversation

ralphbean
Copy link
Member

This will let us test against the currently released version of operator
courier on pypi, as well as whatever they have cooking in their master branch.

An upcoming release (2.0.0) is going to have a small break in backwards
compatibility, handled here.

This will let us test against the currently released version of operator
courier on pypi, as well as whatever they have cooking in their master branch.

An upcoming release (2.0.0) is going to have a small break in backwards
compatibility, handled here.
pkgs_yaml = self.bundle['data']['packages']
if hasattr(self.bundle, 'bundle'):
# New, op. courier >= 2.0.0
pkgs_yaml = self.bundle.bundle['data']['packages']

Choose a reason for hiding this comment

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

I want to point out that this will only work in the flat directory case. In the operator-registry manifest format, we don't return anything on this field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack - @kevinrizza can you recommend an approach that will handle both cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, here we're always calling _flatten_manifest_structure before accessing the bundle (up in omps/api/v1/push.py).

@kevinrizza
Copy link

If you are only accessing this after flattening it, it should work. But I still want to recommend that we find an alternative solution to finding the package name here (which I assume is the reason you are accessing this bundle data?). I want to caution that there is no guarantee that in future iterations that concept will always exist, and being able to generate the name based on some metadata associated with the build or repository is going to become necessary at some point most likely

tox.ini Outdated
@@ -1,8 +1,11 @@
[tox]
envlist = py36,py37,flake8
envlist = py{36,37}-courier_{released,master},flake8
Copy link
Contributor

Choose a reason for hiding this comment

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

Could py{36,37}-courier_released,flake8} be the default envlist, and *-courier_master be available only through tox -e? All this in order to lower test execution time while developing and calling tox locally. Travis will run all the flavors in the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, although - I'm not sure how to accomplish that in a tox.ini config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
envlist = py{36,37}-courier_{released,master},flake8
envlist = py{36,37}-courier_released,flake8

Will do the job, but README.md will need an update to document that tox -e 'py{36,37}-courier_master' can be used to test with the latest version of Operator Courier. This is required, b/c tox -l and tox -a will not list these environments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack - should be fixed.

@MartinBasti
Copy link
Contributor

Yes, packageName is reason why we are accessing bundle. I'm open to suggestions how to do that in a better way.

@kevinrizza
Copy link

Yes, packageName is reason why we are accessing bundle. I'm open to suggestions how to do that in a better way.

@MartinBasti @ralphbean

Yes, and this is what I have been pointing out and arguing for a while. The fact that it is possible for the Courier to return this data is orthogonal to the objective of Courier -- the fact that it is needed at all is tech debt that all of us should be working on to eliminate. In the future (hopefully soon) it will not even be a requirement that the package name in the package.yaml has to match the repository name in the registry. When a build system builds a docker image, there is nothing in that dockerfile that guides the name of the image it is building. The same concept should exist here.

Right now, since we have no such infrastructure in our build system in place we should be doing two things:

  1. Exploring how we can get to a state where the metadata in the yaml doesn't guide the repository name.

  2. In the meantime, you can do something like parse the package.yaml file at runtime and extract the packageName field from it.

I do not believe the courier should return something like this as part of its apis, and the fact that it did previously is an implementation detail.

This is in order to save time when running tests locally.

Travis should still execute tests against courier released *and* courier master.
@ralphbean ralphbean merged commit a25350b into master Apr 29, 2019
@ralphbean ralphbean deleted the new-operator-courier branch April 29, 2019 13:49
@MartinBasti
Copy link
Contributor

@kevinrizza because format of operators metadata is still not clear, I'd postpone this discussion. We still support v1 API that doesn't care about packageName, so if operators won't have packageName but explicit repository name, OMPS still support this use case.

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

4 participants