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

new-app: validate non-numeric EXPOSE directive when strategy wasn't specified #11687

Conversation

php-coder
Copy link
Contributor

@php-coder php-coder commented Nov 1, 2016

@@ -901,7 +901,7 @@ func optionallyValidateExposedPorts(config *AppConfig, repositories app.SourceRe
return nil
}

if config.Strategy != "docker" {
if len(config.Strategy) > 0 && config.Strategy != "docker" {
Copy link
Contributor

Choose a reason for hiding this comment

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

there are cases where the strategy is not explicitly specified but new-app detects the build type as dockerfile because a Dockerfile is present in the repo, is this going to handle that properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in this case the type will be docker (and not dockerfile). In other words, empty strategy is the same as docker strategy.

@php-coder
Copy link
Contributor Author

flake #10566
[test]

1 similar comment
@php-coder
Copy link
Contributor Author

flake #10566
[test]

@stevekuznetsov
Copy link
Contributor

Are you seeing those flakes on Jenkins? Otherwise, that is an abuse of the flake tagging feature and you should not be using that to get new tests...

@bparees
Copy link
Contributor

bparees commented Nov 1, 2016

lgtm [merge]

@bparees bparees self-assigned this Nov 1, 2016
@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 1, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10929/) (Image: devenv-rhel7_5291)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 1433dda

@php-coder
Copy link
Contributor Author

php-coder commented Nov 1, 2016

flake #11661
[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 1433dda

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10929/) (Base Commit: 3b97ee7)

@openshift-bot openshift-bot merged commit 246abf9 into openshift:master Nov 1, 2016
@php-coder php-coder deleted the bz1365986_exposed_ports_followup_fix branch November 1, 2016 21:52
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