Ignore extra files in manifest dir #137
Conversation
openshift-ci-robot
added
the
size/XL
|
@kevinrizza tests fail |
operatorcourier/identify.py
Outdated
| logger.error(msg) | ||
| raise OpCourierBadArtifact(msg) | ||
| else: | ||
| return "" |
There was a problem hiding this comment.
What happen when operatorArtifact is not a dict?
Now it will return None. Should it raise an error instead?
Can we instead of empty string use None as representation that there is no operator_artifact?
operatorcourier/build.py
Outdated
| @@ -33,6 +35,10 @@ def _updateBundle(self, operatorBundle, file_name, yaml_string): | |||
| # Determine which operator file type the yaml is | |||
| operator_artifact = self._get_field_entry(yaml_string) | |||
There was a problem hiding this comment.
Could we just do this here instead of rewriting all the code?
try:
yaml_type = identify.get_operator_artifact_type(yamlContent)
except OpCourierBadArtifact as e:
# no valid artifact skipping update
logger.debug(...)
return operatorBundleThere was a problem hiding this comment.
My concern is that a future update is going to force us to support all files and actually bundle them. In that case, it seems like this method shouldn't return an error.
There was a problem hiding this comment.
I don't see difference in code flow. If get_operator_artifact_type doesn't recognize type, instead of return just bundle file anyway with a generic method.
There was a problem hiding this comment.
I think doing something like that is really twisting the purpose of a lot of these abstractions. The problem is that build used to just mean create a dictionary that includes arrays of these files, Now we are still doing that and then trying to apply a directory structure on top of it by unraveling the dictionary.
All this pr was intended to do is ignore extra files by not throwing an error. I do not see the value of continuing to throw an error here if we want to always be able to call this method on any arbitrary file. If we don't want to be concerned with "Is this None, EmptyString, or a real result" then it seems like the correct thing to do is create an enumeration with all the possible file types and include an item like "None of the Above" that returns when it isn't one of our expected files. And anywhere that calls that method needs to deal with that case as well.
There was a problem hiding this comment.
When function cannot parse artifact_type raising exception is IMHO the best thing what can it do in python. You got clear message that function failed and you can catch that exception and modify behavior of upper levels of code accordingly.
Why a some sub-sequential function cannot fail? Build method can catch exception and does different operation, so build method itself will never fail.
I'm fine with returning None if artifact_type cannot be parsed but it will adds there unnecessary if artifact_type is None lines
There was a problem hiding this comment.
I agree that the difference between returning none or empty string is problematic, so instead I have updated the method to have the following behavior:
Return the manifest file type if it can be determined
Return special Unknown type if it can't be determined
Throw an error if the yaml is invalid
In that way, all callers are forced to handle any behavior based on what the file type is (specific or unexpected) and have specific knowledge that the yaml is valid.
There was a problem hiding this comment.
Great work Kevin, although I do have one concern - it seems like only the CRDs, CSVs, and Packages will be placed in the zip that is pushed to Quay. Should the yamls that OLM supports be pushed as well?
Seems like it would be extremely difficult to keep these in sync. In that case, it might just be easier for us to bundle any yaml file that we find in the directory. |
* Update the identifier to return special unknown type when parsing unexpected yaml file * Update callers to ignore file when filetype returned is unknown * Update tests to reflect this change
kevinrizza
force-pushed
the
ignore-extra-files
branch
from
487c1c6
to
998c58a
Compare
|
@awgreene @MartinBasti Please take another look, I updated the identify method to include a new result I am going to create a followup pr to include the other OLM types that we need to support. |
|
Closing this pr in favor of #141 due to an issue with the CI configuration in Travis |
file (i.e. non csv/crd/package)