Skip to content
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

Add a deps target for CI #1262

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Add a deps target for CI #1262

merged 1 commit into from
Apr 27, 2022

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Apr 27, 2022

This target will always try to upgrade the packages in the system with
latest versions from our repo

Signed-off-by: Itxaka igarcia@suse.com

@Itxaka Itxaka requested review from mudler and davidcassany April 27, 2022 09:48
Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

LGTM, just a little leftover I think

@kkaempf
Copy link
Contributor

kkaempf commented Apr 27, 2022

Not sure if this is the right approach. The luet installs might also override system binaries (coming e.g. from RPM).
While this PR solves one problem, I fear it might trigger many others.

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 27, 2022

Not sure if this is the right approach. The luet installs might also override system binaries (coming e.g. from RPM).
While this PR solves one problem, I fear it might trigger many others.

I guess this affects building on OBS? How does obs tackles dependency? Not calling luet install, Im sure.

Well, we can do something else, keep the make deps as usual, and have a make deps_ci which does what this patch does. Would that work for you @kkaempf ?

@kkaempf
Copy link
Contributor

kkaempf commented Apr 27, 2022

I guess this affects building on OBS?
No, not at all.
This affects a local system where binaries are installed from RPM or DEP. Just 'blindly' calling "luet install ..." now likely clobbers these installs.

Well, we can do something else, keep the make deps as usual, and have a make deps_ci which does what this patch does.

That should work, thanks.

@davidcassany
Copy link
Contributor

Not sure if this is the right approach.

On second thought I think this a good point. We should limit this to the toolchain we provide in packages/toolchain so I would exclude jq from this PR.

The luet installs might also override system binaries (coming e.g. from RPM).

This is a good point I think it should be considered a bug in our toolchain packages, the luet packages should not interfere with whatever the system provides. However this is hard to solve in a sane way, a common solution would be to set luet packages to install under /usr/local/ buit this collides with our immutable layout approach. So I am not sure there is any other way to make sure we minimize the use of luet packages and ensure those are not provided neither conflict with the system.

This target will always try to upgrade the packages in the system with
latest versions from our repo

Signed-off-by: Itxaka <igarcia@suse.com>
@Itxaka Itxaka changed the title Alwasy upgrade deps Add a deps target for CI Apr 27, 2022
@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 27, 2022

ok, this should work, this only runs on our CI and tries to upgrade all packages in the deps. Should be safe enough to run only on our ci while leaving the make deps target untouched so it can be used locally or on OBS.

@Itxaka Itxaka requested a review from davidcassany April 27, 2022 10:19
Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Looks good

@Itxaka Itxaka enabled auto-merge (squash) April 27, 2022 11:28
@Itxaka Itxaka disabled auto-merge April 27, 2022 11:31
@Itxaka Itxaka merged commit 67ebac9 into rancher:master Apr 27, 2022
@Itxaka Itxaka deleted the ci_upgrade_deps branch April 27, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants