-
Notifications
You must be signed in to change notification settings - Fork 11
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
Test self-built rook containers also with SES #229
base: master
Are you sure you want to change the base?
Conversation
36e716a
to
5c556b6
Compare
@jhesketh Please have a look |
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'm a little unsure if building from git is the right place. We may carry patches and other nuances in OBS. I'm inclined to say that the correct method would be to:
- branch the devel or OBS project into your home repo,
- pull in your git branch of github.com/SUSE/rook,
- build in OBS
- Point rookcheck at the built containers
This will ensure we are building in the same manner as the proper pipeline.
Thoughts?
My point is to test a self-built rook-container (this might be a branch of SUSE/rook or also rook/rook) against SES. |
Why would it take ages? It's a few more manual steps, but it at least allows us to use the same tooling as the release pipeline. |
This feature is really all about to test changes while the development process or e.g. bug hunting. This means adding additional steps makes it more complicated and adds more possible places of errors. Is this OK for you? |
Well it adds a bit of complication and also the possibility for something to work in this environment but not OBS. Having said that, I'm fine with this. If you can fix up the tox issues we can merge it :-) |
Signed-off-by: Stefan Haas <[email protected]>
Fixed! Thank you. |
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.
A couple of extra things I noticed/nits. However I know we've done a lot of revisions on this so if you'd like we can merge it and follow them up in a new PR?
# The target version of SES to install. This must match a key below that | ||
# contains the repositories and yaml substitutions. | ||
|
||
target = "devel7" | ||
|
||
[ses.ses7] | ||
rook_ceph_chart = "registry.suse.de/suse/sle-15-sp2/update/products/ses7/charts/rook-ceph:latest" | ||
ceph_image = "registry.suse.de/devel/storage/7.0/containers/ses/7/rook/ceph:latest" |
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 shouldn't be devel?
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.
Actually thinking about this more, why is the ceph_image part of this change? It seems unrelated to providing a custom rook_image. It could be a separate change though.
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.
When using self-build rook-images I do not want to use the rpm-package with the upstream or SES based yaml-files and helm charts. I want to use the ones out of my set rook-branch. Therefore I need to know which images for "fixing" the yaml-files.
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.
Right, I see now.
We could use the yaml_substitutions to achieve this, but that might be too complex? Maybe an example file would help if we did though.
If not, it might be worth noting that this is only used when building from git, and that the below yaml_substitutions are only used when /not/ building from git.
@@ -0,0 +1,21 @@ | |||
--- |
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.
(nit) Would prefer this file to be renamed to avoid the confusion between ses and upstream. Perhaps playbook_rook_ses_custom_image.yaml
?
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 am fine with that change, too. My intention was to use the naming-scheme which is already present (playbook_rook_upstream.yaml
).
This PR adds the possibility to test self-built rook containers against SES
Signed-off-by: Stefan Haas [email protected]