Skip to content

getEnvInjectAction() change#13

Open
StefanSpieker wants to merge 1 commit intojenkinsci:masterfrom
StefanSpieker:getEnvInjectAction
Open

getEnvInjectAction() change#13
StefanSpieker wants to merge 1 commit intojenkinsci:masterfrom
StefanSpieker:getEnvInjectAction

Conversation

@StefanSpieker
Copy link
Contributor

A new pull request which has only the change getEnvInjectAction. But from my point of view it will not really change the interface. It still returns an action, although it uses internally a List of Actions or classes which extend the Action class.

@oleg-nenashev oleg-nenashev self-requested a review September 12, 2019 15:23
@oleg-nenashev
Copy link
Member

The real difference here is that getAllActions() takes both persisten and transient actions while getActions() takes only persisted ones. So the change may cause the performance degradation. EnvInjectAction is always persisted in the current implementatinos.

Not that I am against the change, but it is rather a new feature than a code cleanup

@oleg-nenashev
Copy link
Member

@StefanSpieker what do you think?

@StefanSpieker
Copy link
Contributor Author

I was not aware of that fact. I think we should then just close the PR. In jenkins there is no not deprecated functionality for that. Was it intended to perspectively remove this functionality?

@oleg-nenashev
Copy link
Member

jenkinsci/jenkins#4211 from @KostyaSha somewhat improves this situation by offering explicit API for persisted actions.

Until that PR is integrated, I would prefer not to integrate this change. Performance is probably more important than the code cleaningness. But it would be great to add a TODO comment so that we eventually adopt a new API method

@oleg-nenashev
Copy link
Member

So jenkinsci/jenkins#4211 was closed @StefanSpieker. Should we recover it?

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.

2 participants