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

Flip inheritance to allow yaml to override parent #623

Closed
wants to merge 7 commits into from

Conversation

Vlatombe
Copy link
Member

The current inheritance is counter intuitive (as indicated in the test I changed). I think the yaml fragment should always override the parent definition.

cc @carlossg wdyt?

@jglick
Copy link
Member

jglick commented Oct 17, 2019

cascadingDelete timeout, probably unrelated?

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Do not understand the implications well enough to review.

@carlossg
Copy link
Contributor

It makes sense, but is the parent yaml merged with the child yaml?
otherwise parent yaml would override children UI config

@Vlatombe
Copy link
Member Author

Fair point, I guess it could happen if parent defines yaml but children doesn't. But I believe the current 'yaml merge strategy' already provides enough control as one could easily provide a bogus yaml kind: Pod using override strategy.

The scenario where I encounter problems with the current implementation is Defaults Provider Template Name points to a pod template that defines a jnlp image, and this prevents a pod template defined by yaml (with podTemplate step) to use another jnlp image.

@Vlatombe Vlatombe added the enhancement Improvements label Oct 17, 2019
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Breaks cascadingDelete, not yet sure why. Somehow combine does not work correctly on serviceAccountName.

@jglick
Copy link
Member

jglick commented Oct 23, 2019

A flake in the last build, to be diagnosed by jenkinsci/jenkins#4226.

jglick added a commit to jglick/kubernetes-plugin that referenced this pull request Oct 23, 2019
…es not work.

Also needed to skip pods using nodeAffinity.
Copy link
Contributor

@kerogers-cloudbees kerogers-cloudbees left a comment

Choose a reason for hiding this comment

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

I think this would resolve the issue found trying to run a windows container which was having its image replaced by the default jnlp alpine image.

@jglick
Copy link
Member

jglick commented Oct 23, 2019

combine does not work correctly on serviceAccountName

Because the combination logic currently pays attention only to the deprecated serviceAccount. We need to map these to serviceAccountName automatically, or something like that.

@Vlatombe Vlatombe closed this Nov 6, 2019
@Vlatombe Vlatombe reopened this Nov 6, 2019
@Vlatombe
Copy link
Member Author

I don't think this can be merged because of backward compatibility concerns.

@Vlatombe Vlatombe closed this Oct 18, 2021
@jglick
Copy link
Member

jglick commented Oct 18, 2021

Agreed. We could deprecate the current inheritance attributes and introduce replacements that work more naturally; or just discourage users from relying on this feature, using libraries or whatever else to prepare YAML and pass it as a complete literal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants