[JIRA] (JENKINS-58450) CHANGE_FORK doesn't point to the user/repo when the fork name differs from the upstream name

已查看 0 次
跳至第一个未读帖子

bitwiseman@gmail.com (JIRA)

未读,
2019年7月18日 11:36:032019/7/18
收件人 jenkinsc...@googlegroups.com
Liam Newman updated an issue
 
Jenkins / Bug JENKINS-58450
CHANGE_FORK doesn't point to the user/repo when the fork name differs from the upstream name
Change By: Liam Newman
Summary: CHANGE_FORK doesn't point out to the user/repo when the fork name differs from the upstream name
Add Comment Add Comment
 
This message was sent by Atlassian Jira (v7.11.2#711002-sha1:fdc329d)

jglick@cloudbees.com (JIRA)

未读,
2019年8月7日 15:13:022019/8/7
收件人 jenkinsc...@googlegroups.com
Jesse Glick commented on Bug JENKINS-58450
 
Re: CHANGE_FORK doesn't point to the user/repo when the fork name differs from the upstream name

For consistency, a variable should always include both the account/organization and the repository name, even in the normal case that the fork has the same name as the original repository. Since CHANGE_FORK has been only an account/organization for over two years now, we would risk breaking existing scripts by changing its meaning now. Would need to introduce a new variable name, which means

  • (scm-api) introducing String SCMHeadOrigin.Fork.getFullName(), I guess defaulting to null
  • (branch-api) making BranchNameContributor bind it when set, say to CHANGE_FORK_FULL
  • (github-branch-source et al.) overriding the new method in, e.g., PullRequestSCMHead

VictorMartinezRubio@gmail.com (JIRA)

未读,
2019年8月8日 14:28:022019/8/8
收件人 jenkinsc...@googlegroups.com

I've got a bit skeptical about the backward compatibility, as the current implementations for already provides the account|org/repo format:

The proposal of creating a new env variable will provide a misleading since CHANGE_FORK and CHANGE_FORK_FULL env variables will behave differently based on the specific branch source plugin and therefore might confuse. Besides, the API is the one which specifies what it should be done, therefore the implementation should proceed with the requirement.

In other words:

  • github-branch-source  will contain two env variables: CHANGE_FORK=account CHANGE_FORK_FULL=account/repo
  • bitbucket-branch-source: CHANGE_FORK=account/repo CHANGE_FORK_FULL=account/repo
  • gitlab-branch-source: CHANGE_FORK=account/repo CHANGE_FORK_FULL=account/repo

As the implementation is not right, it means it's required to touch even in the API to solve it which it might be awkward. For sure it's a breaking change, but people should already be aware the implementation itself should be account/repo as already noticed in the docs. So they should not be surprised about it, although adding an entry in the release notes with the implication of those changes might be enough.

Besides, I've got a question, the current implementation for  CHANGE_FORK is invalid when the fork name differs from the upstream name, therefore how do we encourage people to avoid using it and use CHANGE_FORK_FULL? 

 

 

bitwiseman@gmail.com (JIRA)

未读,
2019年8月8日 20:26:032019/8/8
收件人 jenkinsc...@googlegroups.com

Jesse Glick

As I've noted, PR-235 is not perfect.  I agree with you that the right solution in the long term is involves fixing the underlying API/data.

However, in the short term I'm inclined to take PR-235 and open a new JIRA to address the API issue when time permits.  

I do have concerns as noted below, but I don't think they are sufficient to block us for taking PR-235. 

Pros

Concerns

  • New value format changes depending on forked repo name - old format is always owner-name, new format is owner-name/forked-repo-name, but only when the the fork repo name differs from the root.  Consumers of CHANGE_FORK must now check if it contains a slash to know which format to use.
  • Changes the data stored in underlying object not just the exposed CHANGE_FORK - despite this being valid based on the API description, all consumer of origin will be effected and will need to be tested as correctly handling this change.

Related notes:
CHANGE_FORK is exposed here:
https://github.com/jenkinsci/branch-api-plugin/blob/c4d394415cf25b6890855a08360119313f1330d2/src/main/java/jenkins/branch/BranchNameContributor.java#L72-L74

 

jglick@cloudbees.com (JIRA)

未读,
2019年8月13日 12:05:022019/8/13
收件人 jenkinsc...@googlegroups.com

I had missed that the Javadoc actually encourages an implementation to switch formats, meaning the current PR follows the API, as confusing and unfriendly as this is. I do not know enough about BitBucket or GitLab to comment on their behaviors. In light of all this, I guess the PR’s behavior is tolerable enough, though it will certainly merit a release note, and https://github.com/jenkinsci/branch-api-plugin/blob/db3c6487e3fa9f0214b6e50e5808eba84b531aff/src/main/resources/jenkins/branch/BranchNameContributor/buildEnv.properties#L32 must be corrected to explain that the specific format may not only vary from one SCM/provider to the next, but may vary even across different cases in a single provider.

JENKINS-58716 can I think work with a variable format, though it is not ideal: use the full string if it contains a / and otherwise append the inferred repository name.

bitwiseman@gmail.com (JIRA)

未读,
2019年8月23日 18:22:022019/8/23
收件人 jenkinsc...@googlegroups.com
Liam Newman updated Bug JENKINS-58450
 

Thanks Jesse Glick.  I've merged.  This definitely needs a release note.  It think it deserves a minor version bump as well.

Change By: Liam Newman
Status: In Progress Fixed but Unreleased
Resolution: Fixed
回复全部
回复作者
转发
0 个新帖子