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

Move around due to new directories layout #273

Merged
merged 2 commits into from
Jan 16, 2018

Conversation

ScorpioCPH
Copy link
Member

@ScorpioCPH ScorpioCPH commented Jan 9, 2018

As proposed here: #261
This PR move folders around to match new directories layout with nothing changed.

@jlewi PTAL, Thanks!


This change is Reviewable

@k8s-ci-robot
Copy link

Hi @ScorpioCPH. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Jan 9, 2018

Coverage Status

Coverage remained the same at 30.446% when pulling 14e4ac2 on ScorpioCPH:move-around into 9c93d5f on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

/ok-to-test

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

Looks good. Just waiting to see if the tests pass.

@ScorpioCPH
Copy link
Member Author

@jlewi Sorry, tf-k8s-presubmit failed, but i can't see the err logs.

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage remained the same at 30.446% when pulling 0bf0681 on ScorpioCPH:move-around into 9c93d5f on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

I don't actually see the build/ dir in the repo so I think the files build/images/... are actually missing from the PR. Looks like build/ is ignored by .gitignore.

@jlewi
Copy link
Contributor

jlewi commented Jan 10, 2018

I was looking around to see what other projects do.
prow has in images directory in prow/cmd.

I actually like that as it means the image leaves close to the directory. Could we do the same thing and
move build/images/tf_operator/Dockerfile -> pkg/cmd/images/tf_operator
?

@ScorpioCPH
Copy link
Member Author

@jlewi Maybe it's a little weird putting some Dockerfiles in cmd, and pkg is directory for library in golang, i think there is no pkg/cmd sub directory.

Like kuberneres repo, there is a build directory for Dockerfiles. I will take a look at gitignore.

And another thing: can we attach the building logs in the details link? so PR author can debug the building errors.

@jlewi
Copy link
Contributor

jlewi commented Jan 11, 2018

I don't feel too strongly about the location. If you think build is best I can live with that.

Short answer is the test infrastructure is a pain point and its something I plan on fixing.

  • We will copy logs to Gubernator
  • We will open up the Argo UI

But this is blocked on migrating off Airflow to Argo. Its not worth fixing with Airflow since I want to get rid of Airflow.

@ScorpioCPH ScorpioCPH force-pushed the move-around branch 2 times, most recently from 0a33567 to f0b63ee Compare January 11, 2018 06:58
@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage remained the same at 30.624% when pulling f0b63ee on ScorpioCPH:move-around into 7ddafd5 on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage remained the same at 30.624% when pulling f0b63ee on ScorpioCPH:move-around into 7ddafd5 on tensorflow:master.

@ScorpioCPH
Copy link
Member Author

@jlewi Hi, is there any thing i can help about e2e stuff? I'm happy to take a look at this :)

@jlewi
Copy link
Contributor

jlewi commented Jan 12, 2018

We need to get #293 fixed before we can trust the tests again.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 31.714% when pulling 5639c39 on ScorpioCPH:move-around into 98a34a1 on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage remained the same at 31.609% when pulling 47aec7d on ScorpioCPH:move-around into 98a34a1 on tensorflow:master.

@ScorpioCPH
Copy link
Member Author

@jlewi Tests passed, PTAL, thanks!

@jlewi
Copy link
Contributor

jlewi commented Jan 15, 2018

Tests did not pass. Due to #315 you need to actually click on the details for tf-k8s-presubmit. There you see there is 1 test failure which should be fixed by #308.

@jlewi
Copy link
Contributor

jlewi commented Jan 16, 2018

Head should be fixed. Can you sync and push so the tests rerun?

@ScorpioCPH
Copy link
Member Author

/test all

@coveralls
Copy link

coveralls commented Jan 16, 2018

Coverage Status

Coverage increased (+0.1%) to 31.541% when pulling 8863936 on ScorpioCPH:move-around into b97dfc7 on tensorflow:master.

@jlewi jlewi merged commit 357a509 into kubeflow:master Jan 16, 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.

4 participants