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

[JENKINS-41124] Move name mangling support responsibility to AbstractFolder #72

Merged
merged 13 commits into from Jan 22, 2017

Conversation

@stephenc
Copy link
Member

commented Jan 18, 2017

See JENKINS-41124
Downstream of jenkinsci/cloudbees-folder-plugin#82

Additional test cases:

  • Need a test that verifies after migration and then a restart that all is still as expected.
  • Need a test that verifies after a migration and then a reload that all is still as expected.
  • Need a test that verifies reloading the folder and not all of Jenkins leaves all still as expected.
  • Need a test that verifies after a migration and thene a restart and then a reload that all is still as expected.
  • Need a set of all those migration tests using the migration from a 2.0.0 pristine set of items
  • Need a set of all those migration tests using the migration from a 1.x set of items that was migrated to 2.0.0 and then lands on this set
  • Need a set of migration tests that verifies a pristine instance is in the same state as a migrated instance

@reviewbybees esp @jglick @michaelneale

[JENKINS-41124] Move name mangling support responsibility to Abstract…
…Folder

- Needs more tests... like way more...
- [ ] Need a test that verifies after migration and then a restart that all is still as expected.
- [ ] Need a test that verifies after a migration and then a reload that all is still as expected.
- [ ] Need a test that verifies after a migration and thene a restart and then a reload that all is still as expected.
- [ ] Need a set of all those migration tests using the migration from a 2.0.0 pristine set of items
- [ ] Need a set of all those migration tests using the migration from a 1.x set of items that was migrated to 2.0.0 and then lands on this set
@reviewbybees

This comment has been minimized.

Copy link

commented Jan 18, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@stephenc stephenc force-pushed the stephenc:jenkins-41124 branch from 66a8fad to 4923bd1 Jan 18, 2017

@stephenc stephenc force-pushed the stephenc:jenkins-41124 branch from 58e3098 to 61e5af5 Jan 18, 2017

stephenc added 4 commits Jan 18, 2017
[JENKINS-41124] Lots of data migration tests
Need the restart tests also, but this at least calms my fears
@michaelneale

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

I tried the latest snapshot out in blue ocean but with no luck 🐛:

[ERROR] Failed to execute goal org.kohsuke:access-modifier-checker:1.7:enforce (default-enforce) on project blueocean-events: Failed to enforce @Restricted constraints: JAR entry META-INF/annotations/org.kohsuke.accmod.Restricted not found in /Users/michaelneale/.m2/repository/org/jenkins-ci/plugins/branch-api/2.0.2-SNAPSHOT/branch-api-2.0.2-SNAPSHOT.jar -> [Help 1]

branch api in this case was: 2.0.2-20170119.100545-2

previously: 2.0.2-20170118.220727-1 which did work

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@reviewbybees this should be ready for review now

@michaelneale

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

@stephenc do you have a published snap to try?

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@michaelneale I'll re-publish in a few minutes... just sorting out a copy of the bits that were pulled from the release UC so that JENKINS-41207 can at least not be running buggy beta versions

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

@michaelneale 2.0.2-20170119.233611-4 should be the branch-api snapshot. 5.17-20170119.163248-6 for folders

@michaelneale

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

It builds ok, URIS look good, but PRs don't quite show up right (I think they are ok but not in blue ocean).

EDIT: in a previous version the data for PRs looked a bit different...

When I ran a re-index I saw this (likely irrelevant):

SEVERE: Executor threw an exception
java.lang.NoSuchMethodError: hudson.plugins.git.GitSCM.setBrowser(Lhudson/plugins/git/browser/GitRepositoryBrowser;)V
	at org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.build(GitHubSCMSource.java:818)
	at jenkins.scm.api.SCMSource.build(SCMSource.java:778)
	at jenkins.branch.MultiBranchProject.newBranch(MultiBranchProject.java:400)

I would need to ask @vivek for help diagnosing why PRs don't look right now.

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

No such method indicates git plugin is not 2.6.2 but 3.0.0 or 3.0.1 (you want 3.0.2 or 3.0.3)

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

That No Such Method error will cause some branches to fail to index correctly IIRC

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2017

Created JENKINS-41244 to track

@jglick
jglick approved these changes Jan 20, 2017
@@ -205,7 +205,7 @@ public MultiBranchProjectFactoryDescriptor getDescriptor() {
* @param source a repository
* @return criteria for treating its branches as a match
*/
@NonNull
@CheckForNull

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

How on earth is this related? And the Javadoc does not describe what a null return value would mean.

/**
* Encodes names that are not nice so that they are safe to use as url path segments.
* We don't want to do a full url encoding, only replace problematic names with {@code %} escaped variants so
* that when double encoded by Stapler etc we bypass issues.

This comment has been minimized.

Copy link
@jglick
package jenkins.branch;

/**
* Encodes names that are not nice so that they are safe to use as url path segments.

This comment has been minimized.

Copy link
@jglick
return "%00";
}
if (".".equals(name)) {
// just enough escaping to bypass the URL segment meaning of ".."

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

. I suppose you mean.

}
if ("..".equals(name)) {
// just enough escaping to bypass the URL segment meaning of ".."
return "%2E.";

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

I wonder if some web servers will treat an attempt to visit such URLs as an attempted hack and block them. I know we have had problems whereby valid URLs like http://jenkins/descriptorById/whatever/thing/foo%2Fbar/ correctly call Whatever.DescriptorImpl.getThing("foo/bar") in most cases but in some configurations of Tomcat produce a 404 for supposed security reasons.

if ("%2E.".equals(name)) {
return "..";
}
if (name.indexOf('%') == -1) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

move this normal case to the top for efficiency

for (char c : name.toCharArray()) {
switch (matchIndex) {
case 1:
if (c == '2' || c == '3' || c == '5') {

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

Is this machine-generated code?!

This comment has been minimized.

Copy link
@stephenc

stephenc Jan 21, 2017

Author Member

Are you calling me a machine?!

@@ -31,6 +31,7 @@
import hudson.model.ListView;
import hudson.model.Result;
import hudson.model.TopLevelItem;
import hudson.util.StreamTaskListener;

This comment has been minimized.

Copy link
@jglick
@@ -74,11 +74,11 @@ public void locate() throws Exception {
stuff.scheduleBuild2(0).getFuture().get();
r.waitUntilNoActivity();
showComputation(stuff);
FreeStyleProject master = r.jenkins.getItemByFullName("stuff/dev-flow.bn29c1", FreeStyleProject.class);
FreeStyleProject master = r.jenkins.getItemByFullName("stuff/dev%2fflow", FreeStyleProject.class);

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

%2F or %2f?

r.addStep(new Statement() {
@Override
public void evaluate() throws Throwable {
OrganizationFolder foo = (OrganizationFolder) r.j.jenkins.getItem("foo");

This comment has been minimized.

Copy link
@jglick

jglick Jan 20, 2017

Member

getItemByFullName takes a type param

@jglick
jglick approved these changes Jan 22, 2017
@@ -203,7 +203,7 @@ public MultiBranchProjectFactoryDescriptor getDescriptor() {
* Defines how to decide whether or not a given repository should host our type of project.
*
* @param source a repository
* @return criteria for treating its branches as a match
* @return criteria for treating its branches as a match or ({@code null} if all branches match)

This comment has been minimized.

Copy link
@jglick

jglick Jan 22, 2017

Member

misplaced lparen

@stephenc stephenc merged commit 05030c1 into jenkinsci:master Jan 22, 2017

1 check was pending

Jenkins Jenkins is validating pull request ...
Details

@stephenc stephenc deleted the stephenc:jenkins-41124 branch Jan 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.