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 Zeitwerk loader support #892

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

ofedoren
Copy link
Member

Not sure what's the right branch for opening PRs against :/

Not mergeable until deps are merged.

@ofedoren ofedoren force-pushed the feat-support-zeitwerk branch 2 times, most recently from 794ce62 to 91b8996 Compare July 30, 2024 10:07
@ofedoren
Copy link
Member Author

ofedoren commented Sep 3, 2024

Requires 4 plugins: tasks, rex, ansible, katello, I suggest revisiting this plugin after the deps are merged.

Not quire ready, probably some additional changes will be made.

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.

We'll need to drop the temporary bits.

.github/workflows/ruby_tests.yml Outdated Show resolved Hide resolved
gemfile.d/foreman_ansible.rb Outdated Show resolved Hide resolved
gemfile.d/foreman_remote_execution.local.rb Outdated Show resolved Hide resolved
gemfile.d/foreman_tasks.local.rb Outdated Show resolved Hide resolved
gemfile.d/katello.rb Outdated Show resolved Hide resolved
@ofedoren
Copy link
Member Author

Since it's green now, should I remove the last commit? Or leave it for you to decide when there is more appropriate timing?

@evgeni
Copy link
Member

evgeni commented Sep 12, 2024

I'd remove it, but I am also not the maintainer of this plugin :)

@ofedoren ofedoren force-pushed the feat-support-zeitwerk branch from 6421692 to 0c709ad Compare September 12, 2024 11:42
@jeremylenz
Copy link
Collaborator

@ofedoren is this ready for merge?

@ekohl
Copy link
Member

ekohl commented Sep 12, 2024

This has a similar issue as what theforeman/foreman_virt_who_configure#201 shows. Right now it's only testing with a released version of Katello, but that doesn't contain the Zeitwerk changes. Previously it added a gemfile.d file to use a git version, but now CI is red. If you add that change, katello-devel breaks.

Edit: I'm also entirely unsure how this repo does branching. The default is a v10 branch and there is no concept of master/develop so does that mean it first needs to change the default branch to v11 since it's breaking? That's why I'm not merging this now.

@chris1984
Copy link
Member

This has a similar issue as what theforeman/foreman_virt_who_configure#201 shows. Right now it's only testing with a released version of Katello, but that doesn't contain the Zeitwerk changes. Previously it added a gemfile.d file to use a git version, but now CI is red. If you add that change, katello-devel breaks.

Edit: I'm also entirely unsure how this repo does branching. The default is a v10 branch and there is no concept of master/develop so does that mean it first needs to change the default branch to v11 since it's breaking? That's why I'm not merging this now.

@ekohl we used v10 as the branch name since we needed a new release but we were in between Foreman releases for 6.16

Talking with @ShimShtein he wanted to name the next branch v17 to more match the Satellite releases downstream so we know what version of satellite to attach things to for cherry-picks etc.

We were going to make a new branch after Satellite 6.16 goes GA, but if it's blocking stuff, I can talk to Shim Thursday during our 1:1 and see if we can branch early.

@ShimShtein
Copy link
Member

@ekohl v10 is intended to be used with Foreman 6.12 (without Zeitwerk). I see this change will cause CI failures in the upstream, so let's switch to a new branch and make sure we're off the hook.

Talking with @ShimShtein he wanted to name the next branch v17 to more match the Satellite releases downstream so we know what version of satellite to attach things to for cherry-picks etc.

I have also seen your comment about not attaching versions of plugins to downstream products (which is understandable). Any advice on better version numbering? Maybe attaching to the predicted Foreman version? Like calling the next one v13 (as it will go with Foreman 3.13)?

@jeremylenz
Copy link
Collaborator

Any advice on better version numbering? Maybe attaching to the predicted Foreman version? Like calling the next one v13 (as it will go with Foreman 3.13)?

Sure that sounds fine to me, but I think the more fundamental question is why does rh_cloud need to change its default branch every upstream major release? Why not just have a main or develop and then branch the same way as all other plugins?

@ShimShtein
Copy link
Member

ShimShtein commented Sep 17, 2024

Why not just have a main or develop and then branch the same way as all other plugins?

Mainly historical reasons. Originally we were developing a couple of versions in parallel each version for the released and supported Satellite version. Currently it's a bit less needed, although still, having the branch name to indicate what version of the plugin is currently under development sounds like a good idea to me. As I have said to Ewoud, it's a dilemma for me now, as both approaches have their own advantages.

Specifically in the case of Zeitwerk it's good that the PR wasn't merged, since the current branch is intended to be used with Foreman 3.12 which does not have Zeitwerk.

@ofedoren ofedoren changed the base branch from v10 to develop September 19, 2024 13:22
@ofedoren ofedoren force-pushed the feat-support-zeitwerk branch 2 times, most recently from 56e513a to 6a9b229 Compare September 19, 2024 13:45
@ehelms
Copy link
Member

ehelms commented Sep 19, 2024

My 2 cents:

The develop branch should track with Foreman's develop branch. And if a branch is needed to track a released version of the plugin, create that branch. This is the model all the other plugins take, and we should pattern match for continuity, ease of understanding and procedure.

@ofedoren ofedoren force-pushed the feat-support-zeitwerk branch 4 times, most recently from 024897f to 9be502a Compare September 20, 2024 13:38
@ofedoren
Copy link
Member Author

Hmm, 1 of 3 test:foreman_rh_cloud tasks fails for some reason...

@ofedoren
Copy link
Member Author

... but now everything is green ...

@chris1984
Copy link
Member

Thanks @ofedoren

@chris1984 chris1984 merged commit c702bec into theforeman:develop Sep 20, 2024
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants