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

Checking for openshift yaml recursively in the current directory #23

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

rupalibehera
Copy link
Member

checking recursively for openshift.yml and removed the hardcoded path for openshift.yaml

#13

cc: @rawlingsj

try {
new File('.').eachFileRecurse(FILES) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where does FILES come from? I think that needs to be set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes my bad, I missed the import statement import static groovy.io.FileType.FILES. I kinda did a mock testing where I had imported it.

try {
new File('.').eachFileRecurse(FILES) {
if(it.name.endsWith('openshift.yml')) {
def openshiftYaml = it.absolutePath
Copy link
Contributor

Choose a reason for hiding this comment

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

The def openshiftYaml here means the rest of the function cant see it. You can define openshiftYaml variable at the top of the method.

@rawlingsj
Copy link
Contributor

ok thanks @rupalibehera! I added a couple of comments.

FWIW I tested the changes using this inline jenkins pipeline script, you can copy / paste it youself into a new Jenkins pipeline job created via the jenkins UI..

@Library('github.com/rupalibehera/fabric8-pipeline-library@issueid-13')
def dummy
mavenNode {
  ws{
      git 'https://github.com/rawlingsj/t3d.git'
      container(name: 'maven') {
          sh 'mvn clean install'
          def flow = new io.fabric8.Fabric8Commands()
          def rs = flow.isOpenShiftS2I()
          echo "${rs}"
      }
  }
}

@rupalibehera rupalibehera force-pushed the issueid-13 branch 3 times, most recently from b4bd9de to 5d5fa11 Compare January 17, 2017 09:26
@rupalibehera rupalibehera force-pushed the issueid-13 branch 2 times, most recently from ae97bbd to 64ef250 Compare January 19, 2017 12:47
@rupalibehera
Copy link
Member Author

@rawlingsj , I have tested the above PR as suggested by you and it works fine now, let me know if you have anymore reviews on this ?

@rawlingsj
Copy link
Contributor

Could you rebase this PR please @rupalibehera and we'll get it merged in

@@ -498,10 +498,10 @@ def getServiceURL(String serviceName, String namespace = null, String protocol =
}

def isOpenShiftS2I() {
def openshiftYaml = "target/classes/META-INF/fabric8/openshift.yml"
def openshiftYaml = findFiles(glob: '**/openshift.yml')
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if this finds multiple files? Should they all be checked or just the first one as you're doing below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if we will have multiple files, as we can see above in the diff we were earlier just checking for one file in the same location now we are just checking for it recursively in the same directory.

@rupalibehera
Copy link
Member Author

@rawlingsj , I have rebased the PR

@rupalibehera
Copy link
Member Author

@rawlingsj , can we merge this one ?

@rawlingsj
Copy link
Contributor

[merge]

@fusesource-ci fusesource-ci merged commit 894e307 into fabric8io:master Apr 6, 2017
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