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 new API to store input parameter info separately from the execution #3

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

svanoort
Copy link
Member

Very basic version of what I describe in #2 (comment) -- doesn't bother to move some logic to the InputAction that could be.

Attaches information about input prompts on a run to the InputAction (persistently) and provide new APIs to access it. Since most of the external calls to the InputAction are looking to see what input is needed in order to display UI notifications, these APIs should handle this without the risk of deadlock/long wait fetching executions. Should be faster too.

Since older builds won't have the field, we handle null and generate generate the data from the executions when we read or modify them.

CC @jglick to see what you think. I'm probably missing some things here but I want to see if the idea is sound before investing more time to polish it up & test. Should be compatible with #2 as well (just needs that merged in).

There's some sort of small FindBugs complaint about inconsistent synchronization behavior of the run there (shrug), but that's not such a big deal at the moment.

@jglick
Copy link
Member

jglick commented Aug 15, 2016

Still have failures in InputStepTest.test_cancel_run_by_input and InputStepTest.parameter.

this.message = st.getMessage();
this.id = st.getId();
this.parameters = st.getParameters();
this.ok = st.getId();
Copy link
Member

Choose a reason for hiding this comment

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

So what is the purpose of this class? AFAICT we could just save InputStep as is (which is what InputStepExecution does).

Copy link
Member Author

Choose a reason for hiding this comment

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

@jglick Is InputStep safe to use as an API response object on its own? Safe to serialize/deserialize without carrying along context, etc? I didn't see anything explicitly preventing this, but wasn't sure about the implication of passing around & persisting naked steps.

@jglick jglick changed the title [WIP] Add new API to store input parameter info separately from the execution Add new API to store input parameter info separately from the execution Aug 15, 2016
@jglick
Copy link
Member

jglick commented Aug 15, 2016

The idea looks sound, but this PR is incomplete in that it does not show the new API actually being used from the stock UI code. (Obviously a downstream PR showing it being used in stage view would help verify the design as well.)

@jglick
Copy link
Member

jglick commented Aug 15, 2016

Also I would expect to be able to make InputStepExecution.input be transient: loaded from serial form for old builds, but for new builds not used after start, with the metadata being kept in InputAction instead (otherwise we are duplicating everything). That of course means that various methods like parseValue would need to have an InputStep passed in externally. Really all the web methods in InputStepExecution like doProceed should be deprecated, delegating to new methods in InputAction. So I think we are looking at a bigger rewrite.

@jglick
Copy link
Member

jglick commented Jan 29, 2019

See JENKINS-37998.

@jglick jglick marked this pull request as draft January 3, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants