[on-hold] Accepting JEP-202

[jglick]

@kohsuke says

I've assigned Jesse as BDFL Delegate to this JEP.

and

I also think the amount of coding effort that went into this is well past the criteria that I expect for "Accepted"

BTW the set-jep-status script does not work for me:

jep$ (cd jep; ./set-jep-status 202 Accepted)
sed: can't read s/^| Draft :speech_balloon:/| Accepted :ok_hand:/: No such file or directory

CC @carlossg @rtyler

[bitwiseman]

@kohsuke @jglick
Let me start by saying, whether you choose to accept this or not is totally your call.

However, I would suggest that it is premature to move this to "Accepted".

• Carlos did not request a review for Accepted status - A reviewer may decide to review a Draft JEP for Acceptance without the request, but you should inform the sponsor first.

• As noted in JEP process clarification  #76, a JEP does not achieve "Accepted" status based on how much work has been done. Accepted should be used to describe the overall state of the JEP. The justification above is not sufficient (in my opinion) to justify moving this JEP to accepted status.

• Also in JEP process clarification  #76, there is no reason to rush a JEP to "Accepted" status and doing so is actually counterproductive. It can result in people not getting involved, not giving feedback, or even feeling excluded from the design process. In the Draft submission, @oleg-nenashev sounded like he might have more to say about the design after the JEP was approved as Draft. Has the sponsor had time to check back with him to make sure his feedback is

• I'm not an expert in this area, but the Specification section of the JEP seems extremely light and vague to be considered a "scope and design complete" state.

• The PR [JEP-202] Extending VirtualFile jenkins#3302 has to API's set as Beta with notes from Carlos that they may still change depending on feedback. That is in direct contradiction to "Accepted" status.

[jglick]

@carlossg & @oleg-nenashev please add your opinions in writing. Of course we have been in personal communication.

From #76:

the JEP is stable

Not sure exactly what that means but I suppose it is “stable”. Nothing that I know of has been proposed which would materially change the current JEP. @oleg-nenashev suggested that an appendix be added with a list of plugins known to not honor the ArtifactManager / VirtualFile API, which I plan to write soon.

addresses all major design and scope questions

I believe it does.

represents the consensus of the community

To the extent the “community” has had any response at all, beyond @batmat’s tweet getting a bunch of likes and retweets, I suppose it does.

in-progress implementation

The reference implementation is what I would consider “feature complete”. Like any software, we would expect to improve it over time.

the Specification section of the JEP seems extremely light and vague

It certainly does not delve into line-by-line commentary; it outlines the important concepts. The reference implementation is, well, the reference.

APIs set as Beta with notes from Carlos that they may still change depending on feedback

No changes are expected. The intention of the beta annotation here is to allow us to cut software releases (starting with Jenkins 2.118) including newly minted APIs while retaining some oversight. For example, if someone prototypes an Azure implementation and runs into an obstacle which we agree requires the VirtualFile.toExternalURL() method to take a parameter which we did not foresee the need for while working on S3, we would like to be able to simply add the parameter and update existing implementations and call sites without going through the process of deprecating the original overload and leaving it there for another decade. At some point we will decide that it is looking unlikely that such a change would be required, and we can remove the annotation and open up the API for use by unknown parties, making a best effort to maintain compatibility.

[bitwiseman]

@jglick

Thanks for the full and reasoned response. Thanks for taking the time to document your reasoning fully. It addresses my concerns about giving proper consideration to moving to Accepted.

Let me make sure the context of my comments are clear. We just had JEP have to go from Accepted to Draft due to it being moved to Accepted before it was ready. I'm also confused as to why you as Reviewer are pushing to move this JEP to Accepted status.

As Reviewer, it's not your job to make the case for this JEP to be Accepted. Your job is to review the JEP from a critical stand point and consider ways in might not be ready for "Accepted" status. I understand that you are responding to me because this is your PR, but it should really be Carlos making the request, making the case, and responding to my concerns.

Further, since there are concerns voiced by a member of the community (me), that means that this JEP has not reached consensus. One of the concern I had that you did not really address was that "Specification section of the JEP seems extremely light and vague". As Reviewer in the JEP process, your response this kind of concern needs to be "I see you have concerns. That is enough reason for us to wait on Acceptance until the Sponsor, @carlossg, can address them." Then @carlossg and I would figure out what changes if any need to be made.

[oleg-nenashev]

I will provide my feedback tomorrow

[carlossg]

I will start by subscribing what @jglick commented, just adding some more points

Carlos did not request a review for Accepted status

Sorry, my fault if I didn't follow the process exactly, newby here. Let's this serve as an official request to be accepted

there is no reason to rush a JEP to "Accepted" status and doing so is actually counterproductive. It can result in people not getting involved, not giving feedback, or even feeling excluded from the design process

The goal here is definitely to get feedback and improve the design. I can understand there may be some confusion on how to actually do that.
The main focus is to get the needed set of core APIs shipped asap in order to get that feedback and improve the design as needed. There was a thread in the mailing list but that's not going to be as valuable as developers tring to use them. I have separately contacted a couple of possibly interested downstream consumers (cloud providers/repository providers) and will reach to more. IMHO that's when the useful actionable feedback is going to be provided and we now have the @Beta mechanism to allow exactly that.

As Reviewer, it's not your job to make the case for this JEP to be Accepted. Your job is to review the JEP from a critical stand point and consider ways in might not be ready for "Accepted" status

As we worked together on this, Jesse has done that already (a lot! ;) ) during the design and specification to the implementation. These conversations are not public because the origin of the plugin is not OSS but that explains why he has (almost) no concerns about what's stated in the JEP.

One of the concern I had that you did not really address was that "Specification section of the JEP seems extremely light and vague".

Happy to address that if you can provide some detail on what do you think is missing.

[oleg-nenashev]

Not sure exactly what that means but I suppose it is “stable”. Nothing that I know of has been proposed which would materially change the current JEP. @oleg-nenashev suggested that an appendix be added with a list of plugins known to not honor the ArtifactManager / VirtualFile API, which I plan to write soon.

I would vote for doing it before accepting JEP

Since the time-critical part for the core is integrated with @Beta status, IIUC we have some time to process this JEP. I would be really interested to have this API in final state by the next LTS baseline, but we have few months for that.

@oleg-nenashev sounded like he might have more to say about the design after the JEP was approved as Draft. Has the sponsor had time to check back with him to make sure his feedback is

• The JEP does not specify Core API changes, and this is the most important part. "This change adds more methods to the VirtualFile API to support this flow" is not a specification at all
• JEP text still does not explicitly say that there will be no changes for instances which use old filesystem Artifact Manager implementation. From the JEP text it's not clear whether it is in the testing scope
• I am still concerned that API does not support InterruptedException in new and old methods ([JEP-202] Extending VirtualFile jenkins#3302). IMHO it may cause integration issues with plugins like Build Timeout plugins which may want to interrupt the archiving operations. Not a blocker since old APIs didn't do that, but it worths discussion.
• As a potential plugin user, I am concerned that the reference implementation in S3 Artifact Manager does not persist references to buckets/folders within build.xml. Reconfiguration of artifact manager plugin may lead to the data unavailability and to malfunction of BuildDiscarder logic. Since it is a reference implementation, I do not see it as a blocker for accepting JEP. The Factory-based architecture itself supports injecting metadata into runs.

[oleg-nenashev]

Regarding the de-factoVirtualFile implementation, I am fine with that (excepting the InterruptedException concern). But it needs to be documented IMHO.

[bitwiseman]

@carlossg

We're all new to this process. I'm not trying to criticize you. If you'd like, I'd be happy to schedule a some time to chat about it and make sure you have all the information you need.

These conversations are not public because the origin of the plugin is not OSS but that explains why he has (almost) no concerns about what's stated in the JEP.

Some form of those conversations and/or the resulting decisions needs be covered by the JEP.

I have separately contacted a couple of possibly interested downstream consumers (cloud providers/repository providers) and will reach to more. IMHO that's when the useful actionable feedback is going to be provided and we now have the @beta mechanism to allow exactly that.

This is a perfect summation of why I was suggesting to @jglick that this JEP is not ready for acceptance. Both of you are saying "We're still gathering feedback that has the potential to change the JEP and the design." Between this and the concerns @oleg-nenashev voiced, I don't see how this JEP could be considered ready for "Accepted" status.

And let me say again, there is no reason to rush a JEP to "Accepted" status. There is absolutely no advantage. (The only case where it could matter is if there were another JEP in competition with this one, which doesn't apply here at all.)

[carlossg]

@bitwiseman

there is no reason to rush a JEP to "Accepted" status

From the practical point of view I'm fine either way, my main concern was to get the beta APIs in core in order to gather that feedback.
Now that they are merged we are no longer in the chicken and egg scenario of waiting for JEP to be accepted to get changes merged and get feedback, and waiting for feedback to get the JEP accepted

[bitwiseman]

@carlossg
Do you remember who indicated that this JEP needed to be accepted in order for APIs to be published? That's definitely not the case and I want to make sure I've explained it in a way that makes sense to everyone.

[jglick]

I would vote for [adding a list of affected plugins] before accepting JEP

OK, can wait for that if you prefer.

The JEP does not specify Core API changes

Probably it should link to VirtualFile Javadoc (unfortunately not yet rebuilt??) with a list of new methods; as well as StashAwareArtifactManager (not yet merged and so not yet available as online Javadoc, but could perhaps link to sources in the meantime).

I am still concerned that API does not support InterruptedException […which] may cause integration issues with plugins like Build Timeout plugins

As discussed in the core PR, I do not see any evidence that it would (JENKINS-50597, planned for work soon, would verify this), but at any rate the explanation does deserve to be in the Reasoning section.

Also @bitwiseman alerted me to the fact that the discussion of why asRemotable was ultimately removed from the API in favor of solely using toExternalURL should be moved to the Reasoning section, with the Security section only documenting the final state, to follow the standard JEP structure.

I am concerned that the reference implementation in S3 Artifact Manager does not persist references to buckets/folders

It could; there are reasons we declined to do so. Could be discussed under Reasoning, though it is really a policy decision for this particular implementation—I do not think it is essential for readers of the JEP.

[carlossg]

Added more details in #97

[bitwiseman]

#117

[bitwiseman]

Closing this for now. Please reopen when this is ready for review.

[jglick]

This should be mergeable once #117 is. I seem unable to reopen it at the moment.

[bitwiseman]

Per reviewer @jglick, I'm setting this to accepted.