-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use correct version of ubi config file for population #140
Conversation
Tests will be added/modified later. |
ubipop/__init__.py
Outdated
# then it's not a mainline repo, use platform_full_version instead. | ||
version = repo_set.out_repos.rpm.ubi_config_version \ | ||
or repo_set.out_repos.rpm.platform_full_version | ||
right_config = self.ubiconfig_map.get(version, {}).get(config.filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to fulfill following acceptance criteria:
if ubi config is not available for given version, fallback to default version ("7" for ubi7, "8" for ubi8 )
In ubi config repo there will always be 'default' branches ubi7 and ubi8 where you need to fallback if config for required version isn't available. And if I read it correctly, it can assign None to right_config var which will lead ubipop to crash. This can happen for both DOT and mainline repos.
Or there can be some "best effort" mechanism which can select the latest available config < required version, if required config is not available. But this is not needed now I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I wasn't sure what's 'default version'.
I'm still confused about why 'version' could be None? could you elaborate on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, isn't platform_full_version always available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I misread.
yeah, right_config could be None and it leads to the push crashing. I saw that in the first place and I didn't do any exception for it, because I think it should fail in this case, means there's a major error in config repo, we should fail this push.
Do you think we should ignore and continue push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.
Well these default branches (ubi7, ubi8) should be always available.
If they are not available, I'd like to see at least an error message logged so ubipop can populate as much as it can.
5d524ec
to
ce3922a
Compare
I'm not sure why CI passed... |
run tests |
@@ -26,6 +26,10 @@ class RepoMissing(Exception): | |||
pass | |||
|
|||
|
|||
class ConfigMissing(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MissingConfig sounds better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, conforms to existing scheme
# config file could also be missing for specific version, then the | ||
# default config file will be used. | ||
version = ubi_config_version or platform_full_version | ||
right_config = self.ubiconfig_map\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break only on or
, this is hard to read, but not a big deal.
@JayZ12138 |
Yes. |
This resloves release-engineering#138 A new attr is added to Repo class and it will be populated while searching repos. After all config files are loaded, it now creates a config map, which helps find the right config file for the repo to populate.
config file for specific version might not available, it should fall back to load the config file from default version.
run tests |
run tests |
Run tests |
run tests |
@@ -26,6 +26,10 @@ class RepoMissing(Exception): | |||
pass | |||
|
|||
|
|||
class ConfigMissing(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, conforms to existing scheme
This resloves #138
A new attr is added to Repo class and it will be populated while
searching repos.
After all config files are loaded, it now creates a config map, which
helps find the right config file for the repo to populate.