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

MYRIAD-239 Fixed issue with * by updating conf files, added doc #89

Open
wants to merge 2 commits into
base: 0.2.x
Choose a base branch
from

Conversation

hokiegeek2
Copy link
Contributor

comments, and added JUnit test case.

@DarinJ
Copy link
Contributor

DarinJ commented Aug 19, 2016

looks like we have a failed check based off style. Also, not sure if I believe this fixes the issue. Have you tested?

@hokiegeek2
Copy link
Contributor Author

Hey Darin,

Just finished testing MYRIAD-229, moving on to this one and I'll let you
know how AWS testing goes.

--John

On Fri, Aug 19, 2016 at 1:16 PM, Darin [email protected] wrote:

looks like we have a failed check based off style. Also, not sure if I
believe this fixes the issue. Have you tested?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#89 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKSRgboOsnw4i52XIR2lGgTglJGLAXFvks5qheT6gaJpZM4Jm2Ai
.

@DarinJ
Copy link
Contributor

DarinJ commented Aug 24, 2016

Tested yet?

@hokiegeek2
Copy link
Contributor Author

Yep, tested it on our AWS cluster.

--John

On Wed, Aug 24, 2016 at 12:31 PM, Darin [email protected] wrote:

Tested yet?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#89 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKSRgaMJ0fO_UzOOl-xRY1T4yUmjllp9ks5qjHHtgaJpZM4Jm2Ai
.

@bgulla
Copy link
Contributor

bgulla commented Jul 6, 2017

LGTM

@@ -212,6 +212,9 @@ public String getFrameworkName() {
}

public String getFrameworkRole() {
if (frameworkRole.contains("/")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ hokiegeek2, sorry but I think this does not solve the problem. Maybe it's because the code has changed and the patch test does not make sense with the current code. The current problem is that we are treating the default role 'asterisk' as if it were a real role and the behavior has to be as if it were not a role. When the role is 'asterisk', "frameworkRole.contains (" / ")" returns false and never executes the statement "return frameworkRole.replace (" / "," ");", then it continues to fail. I think that the error is produced by the treatment of the role 'asterisc' in "ResourceOfferContainer.setScalarValues", it would be necessary that the behavior of this function with role 'asterisk' was equal to a roleless one. What do you think?

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.

4 participants