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

Remove Test #1097

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Remove Test #1097

merged 1 commit into from
Jun 12, 2018

Conversation

piyush-garg
Copy link
Contributor

@piyush-garg piyush-garg commented Jun 12, 2018

If we keep that test, it will return different URl for
every other CI. If we do some changes in test, it will
be similar to other tests like setting masterUrl and all.
This started happening after PR #1086
Asserting not null and is URL is the only solution
but does not make any difference as we have another tests for this func.

@piyush-garg
Copy link
Contributor Author

This may be because of #1086

@@ -201,6 +201,7 @@ private static String decodeUrl(String url) {
@Test
public void testWithServiceAccount() {
System.setProperty(Config.KUBERNETES_KUBECONFIG_FILE, "/dev/null");
System.setProperty(Config.KUBERNETES_MASTER_SYSTEM_PROPERTY, "https://kubernetes.default.svc/");
Copy link
Member

Choose a reason for hiding this comment

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

Here service account could return the different value hence assertion could vary from environment to environment.

To fix it, Its better way to have deterministic master URL value for SA or let be as this PR to temp fix.

Copy link
Member

Choose a reason for hiding this comment

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

Since its blocking the release. Can we let it go and create another issue to fix this?

cc: @iocanel

If we keep that test, it will return different URl for
every other CI. If we do some changes in test, it will
be similar to other tests like setting masterUrl and all.
This started happening after PR fabric8io#1086
Asserting not null and is URL is the only solution
but does not make any difference as we have another tests for this func.
@piyush-garg piyush-garg changed the title Fix Test failing on CI Remove Test Jun 12, 2018
@piyush-garg
Copy link
Contributor Author

@Vlatombe Would you like to review and provide feedback for this?

Copy link
Contributor

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

LGTM. Indeed, when running as a service account, the service host and port are generally provided as environment variables pointing to the ip/port directly.

@hrishin hrishin merged commit 3b5c121 into fabric8io:master Jun 12, 2018
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.

3 participants