Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refs #31346 - Use Java 11 for Candlepin #372

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Conversation

jturel
Copy link
Contributor

@jturel jturel commented Jan 8, 2021

No description provided.

@jturel
Copy link
Contributor Author

jturel commented Jan 8, 2021

@ekohl @ehelms based on the latest discussion in theforeman/puppet-candlepin#169 is this the correct interpretation / should we do this vs the puppet-candlepin change?

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.

You are missing the dependency chaining. There's 2 ways we can do this. Either here in this class or introduce a parameter to candlepin for the java package (Optional[String[1]] $java_package = undef) and add that to candlepin::install in the right order. I'm slightly leaning to the latter but wouldn't mind the former either. As long as we ensure that the java package is installer before installing candlepin.

@jturel jturel changed the title Use java11 Refs #31346 - Expose java_package parameter Jan 8, 2021
@jturel jturel marked this pull request as ready for review January 8, 2021 19:28
@jturel
Copy link
Contributor Author

jturel commented Jan 11, 2021

[test]

@jturel
Copy link
Contributor Author

jturel commented Jan 11, 2021

@ekohl good to merge? :)

@ekohl
Copy link
Member

ekohl commented Jan 11, 2021

I would like to get a release of puppet-candlepin our and update metadata.json in this module to require at least that since it's a new parameter.

@jturel
Copy link
Contributor Author

jturel commented Jan 11, 2021

Alright, let me know what -candlepin version we end up on and I can update metadata.json here

@ekohl
Copy link
Member

ekohl commented Jan 11, 2021

That would be theforeman/puppet-candlepin#171

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 nit: I think we should change the commit message (and PR title) to state that Candlepin will be using Java 11. Other than that, 👍

@jturel jturel changed the title Refs #31346 - Expose java_package parameter Refs #31346 - Use Java 11 for Candlepin Jan 12, 2021
@jturel
Copy link
Contributor Author

jturel commented Jan 12, 2021

@ekohl updated!

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.

Isn't this Fixes #31346 instead of Refs?

@jturel
Copy link
Contributor Author

jturel commented Jan 12, 2021

Isn't this Fixes #31346 instead of Refs?

The redmine has a collection of PRs to deliver the desired outcome, so that's why I used refs (this PR alone does not get us to where we need to be).

@ekohl
Copy link
Member

ekohl commented Jan 12, 2021

It feels like this is the one that really makes it happen, but I'm fine either way. Let me know if this should be merged as is or you want to change it.

@jturel
Copy link
Contributor Author

jturel commented Jan 12, 2021

It feels like this is the one that really makes it happen, but I'm fine either way. Let me know if this should be merged as is or you want to change it.

I see your point, but I like how Refs communicates that there's more to the story. Let's merge as-is

@ehelms ehelms merged commit f5d4750 into theforeman:master Jan 12, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants