Skip to content

redo multi project fix to punt objectList<->projectList correlation, …#138

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
gabemontero:fix-with-each-groovy-return
Apr 10, 2018
Merged

redo multi project fix to punt objectList<->projectList correlation, …#138
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
gabemontero:fix-with-each-groovy-return

Conversation

@gabemontero
Copy link
Copy Markdown
Contributor

…employ more passive project assignment

Fixes #130

@openshift/sig-developer-experience ptal

this fix addresses issues introduced by the earlier mulit-project fix for single project envs,
but still maintains the ability to have multi-project creates.

on the bright side, this version simplifies to a decent extent the original multi-project fix.

NOTE: I have obtained agreement from 2 upstream users to validate the fix. I'm going the attempt to hold merging until we have validation from them, assuming they respond relatively soon. Also, we do now have a multi-project test case in the PR extended tests, so we should be good there.

In both upstream users cases, they are hitting timing issues between when selectors with DCs are created, and when they attempt to iterate over pods associated with them with openshift.withEach. While they can hit the issue fairly reliably with their Jenkins files, it has been much less frequent with me (I have seen it like once after many runs).

I also found an issue with the openshift.union method ... I plan on creating a new test case for that.

@gabemontero gabemontero added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2018
@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2018
@gabemontero
Copy link
Copy Markdown
Contributor Author

Install origin flake

/retest

@gabemontero
Copy link
Copy Markdown
Contributor Author

Several confirmations from upstream users have arrived confirming that the manually built plugins from this branch has solved the issue for them.

/hold cancel

@openshift/sig-developer-experience bump ... any takers/folks with bandwidth for a review?

I'll wait until sometime after lunch to see if anyone can.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2018
@gabemontero gabemontero added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2018

Result r = new Result(verb);
ArrayList<String> projectNames = new ArrayList<String>();
HashMap<String, String> projectNames = null;// = new HashMap<String, String>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete the comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep should clean that up

String name = obj2MapMetadata.get("name");
r = innerObjectDefAction(verb, obj2, userArgs, ns, r);
projectNames.add(ns);
projectNames.put(kind.toLowerCase().trim()+"/"+name, ns);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this going to have collisions if someone is creating a "build/foo" in two namespaces?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep you are right

alas, more groovy pain to come as I figure out how to assess :-(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok I think the change to address is minor .... will test out in the AM

@gabemontero
Copy link
Copy Markdown
Contributor Author

/hold cancel

@gabemontero
Copy link
Copy Markdown
Contributor Author

stopped ci job

@gabemontero
Copy link
Copy Markdown
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2018
@gabemontero gabemontero removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2018
@gabemontero
Copy link
Copy Markdown
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2018
@gabemontero
Copy link
Copy Markdown
Contributor Author

@bparees comments addressed

@gabemontero
Copy link
Copy Markdown
Contributor Author

separate commit

@gabemontero
Copy link
Copy Markdown
Contributor Author

install origin failure .... more instances of openshift/origin#19293

@gabemontero
Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@gabemontero
Copy link
Copy Markdown
Contributor Author

/retest

Copy link
Copy Markdown
Contributor

@bparees bparees left a comment

Choose a reason for hiding this comment

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

nits and lgtm

projList.put(name, project);
// we hardcode the suffix to 0 since we are only sending
// 1 item from the original nameelist down ... hence the
// suffix is no the index for the first entry in the array
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/nameelist/namelist/
s/no/not? now?/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed thanks

@gabemontero
Copy link
Copy Markdown
Contributor Author

/refresh

@gabemontero gabemontero force-pushed the fix-with-each-groovy-return branch from 738704e to bb5f9fa Compare April 10, 2018 22:09
@gabemontero gabemontero added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2018
@openshift-merge-robot openshift-merge-robot merged commit e0cfbc5 into openshift:master Apr 10, 2018
@gabemontero gabemontero deleted the fix-with-each-groovy-return branch April 11, 2018 15:48
gabemontero pushed a commit to jenkinsci/openshift-client-plugin that referenced this pull request Apr 11, 2018
…y-return

redo multi project fix to punt objectList<->projectList correlation, …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

version 1.0.8 untilEach: watch closure threw an exception: "java.lang.IndexOutOfBoundsException: Index: 1, Size: 1; Index: 1, Size: 1".

4 participants