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

Drop ansible-runner repository management #780

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Sep 16, 2022

The ansible-runner package is now delivered via the Foreman plugins repository.

Requires that ansible-runner be in the plugins repository (theforeman/foreman-packaging#8434)

@ehelms ehelms marked this pull request as ready for review September 16, 2022 03:40
@evgeni
Copy link
Member

evgeni commented Sep 16, 2022

Should we clean up the old repo definition?

@evgeni
Copy link
Member

evgeni commented Sep 16, 2022

And speaking if cleanup, I think f-maintain also has some (wrong) special handling that should be removed.

@ehelms
Copy link
Member Author

ehelms commented Sep 16, 2022

And speaking if cleanup, I think f-maintain also has some (wrong) special handling that should be removed.

I grepped around and couldn't find any. Anything in particular you are thinking it did?

@evgeni
Copy link
Member

evgeni commented Sep 16, 2022

And speaking if cleanup, I think f-maintain also has some (wrong) special handling that should be removed.

I grepped around and couldn't find any. Anything in particular you are thinking it did?

https://github.com/theforeman/foreman_maintain/blob/5f9bf72d5d09f6277d5ee581553f9a7273d8223f/definitions/checks/non_rh_packages.rb#L31

@ehelms
Copy link
Member Author

ehelms commented Sep 16, 2022

https://github.com/theforeman/foreman_maintain/blob/5f9bf72d5d09f6277d5ee581553f9a7273d8223f/definitions/checks/non_rh_packages.rb#L31

I think we still want that? As we deliver ansible-runner in upstream and downstream does as well?

@ehelms ehelms force-pushed the drop-runner-repo branch 2 times, most recently from e4ed4ce to b31aebf Compare September 16, 2022 13:46
@evgeni
Copy link
Member

evgeni commented Sep 16, 2022

No, that check is to allow upstream runner packages in downstream, which is wrong.

@ehelms
Copy link
Member Author

ehelms commented Sep 16, 2022

No, that check is to allow upstream runner packages in downstream, which is wrong.

I still don't think I understand. The only thing we are changing is the version of ansible-runner being packaged up. Why would that affect this logic?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

The code reads well, but there is the part of compatibility. This is breaking, unless we backport the packaging changes to 3.4 (which I'd actually like to see). See https://github.com/theforeman/puppet-foreman_proxy#compatibility as well.

Looks like for now we don't have a breaking change, though #630 will be. If we do go for the breaking change route, I'd like to get a release out with the current changes first.

default: {
fail("Repository containing 'ansible-runner' not known for '${facts['os']['family']}'")
}

Copy link
Member

Choose a reason for hiding this comment

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

I think this breaks lint

Suggested change

@ekohl
Copy link
Member

ekohl commented Sep 16, 2022

I still don't think I understand. The only thing we are changing is the version of ansible-runner being packaged up. Why would that affect this logic?

In foreman-maintain it verifies the vendor on packages, but allows an exception for ansible-runner. That exception becomes redundant once it's packaged by RH and can be dropped.

@ehelms
Copy link
Member Author

ehelms commented Sep 16, 2022

In foreman-maintain it verifies the vendor on packages, but allows an exception for ansible-runner. That exception becomes redundant once it's packaged by RH and can be dropped.

This is what confuses me - it's already packaged by RH.

@ekohl
Copy link
Member

ekohl commented Sep 16, 2022

The exclusion doesn't break anything, it just makes the check weaker than it needs to. So even without this, it can be dropped.

@evgeni
Copy link
Member

evgeni commented Sep 16, 2022

In foreman-maintain it verifies the vendor on packages, but allows an exception for ansible-runner. That exception becomes redundant once it's packaged by RH and can be dropped.

This is what confuses me - it's already packaged by RH.

And that's the wrong part. It allows the non RH (= upstream) package on an RH installs (because someone sometimes forgot to set manage_repo = false)

@ehelms
Copy link
Member Author

ehelms commented Sep 19, 2022

In foreman-maintain it verifies the vendor on packages, but allows an exception for ansible-runner. That exception becomes redundant once it's packaged by RH and can be dropped.

This is what confuses me - it's already packaged by RH.

And that's the wrong part. It allows the non RH (= upstream) package on an RH installs (because someone sometimes forgot to set manage_repo = false)

Done over here -- theforeman/foreman_maintain#644

@ehelms ehelms force-pushed the drop-runner-repo branch 2 times, most recently from e320c4d to 3e5168f Compare September 21, 2022 14:52
@ehelms
Copy link
Member Author

ehelms commented Sep 21, 2022

Looks like for now we don't have a breaking change, though #630 will be. If we do go for the breaking change route, I'd like to get a release out with the current changes first.

We already have a merged breaking change (#776). There are two non-breaking commits that if we wanted we'd have to do a branch and release with those:

Given that would require a branch, I think a release is a separate question from whether this can go in the code base now.

I have included an update to the README.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small notes on the readme bit, otherwise 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
The ansible-runner package is now delivered via the Foreman plugins
repository.
@ekohl ekohl enabled auto-merge (rebase) September 21, 2022 16:26
@ekohl ekohl merged commit 7f6601f into theforeman:master Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants