Don't update api-reference docs if the only changes are the timestamps #16940
Conversation
|
Labelling this PR as size/M |
k8s-github-robot
added
the
size/M
|
Looks fine, script still reuses existing REPO_DIR. Jenkins unit/integration needs to pass in REPO_DIR to be able to run |
| # By now, the contents should be normalized and stripped of any | ||
| # auto-managed content. | ||
| if diff -Bw >/dev/null <(echo "${original}") <(echo "${generated}"); then | ||
| # actual contents same, overwrite generated with original. |
There was a problem hiding this comment.
We should overwrite when the contents are different right?
There was a problem hiding this comment.
aah ok. I see now.
Ignore
There was a problem hiding this comment.
Yes, that's confusing. I copied this pattern from another hack/*.sh.
|
LGTM, thanks! |
nikhiljindal
added
the
lgtm
|
Restart the checks as I just upload the v4 image to gcr. |
|
GCE e2e test build/test passed for commit 358221f. |
|
Looks like this breaks dockerized test. You can repro locally by running |
| @@ -37,12 +47,40 @@ echo "Reading swagger spec from: ${SWAGGER_PATH}" | |||
| mkdir -p $V1_PATH | |||
| mkdir -p $V1BETA1_PATH | |||
|
|
|||
| docker run --rm -v $V1_PATH:/output:z -v ${SWAGGER_PATH}:/swagger-source:z gcr.io/google_containers/gen-swagger-docs:v3 \ | |||
| docker run -u $(id -u) --rm -v $V1_TEMP_PATH:/output:z -v ${SWAGGER_PATH}:/swagger-source:z gcr.io/google_containers/gen-swagger-docs:v4 \ | |||
There was a problem hiding this comment.
The output is supposed to be written to $V1_TEMP_PATH, but when running in dockerized test, nothing is written.
There was a problem hiding this comment.
Ah, well there's your problem. The path that you pass to docker run -v PATH:/output will get resolved on the host, not the container. So it's actually writing to ${KUBE_TEMP} on your machine. See longer explanation here.
The tl;dr is that when you call docker run -v PATH:/containerpath ..., the PATH must be resolvable on the host. To have the above command write output that you can then read here, you have to pass in the host-resolvable path (i.e., use REPO_DIR), and then read that output using the local path (without REPO_DIR).
It's kind of confusing. If you give me a minute, I can send a pr to your branch that should fix.
There was a problem hiding this comment.
Thanks @jlowdermilk! So I didn't understand your comments when I reviewed your PR, now it all makes sense!
As you said, the address I passed to -v will be resolved as a host-resolvable path, then we still need to use ${KUBE_TEMP}, there can be race condition if two dockerized tests are running at the same time. Am I right?
There was a problem hiding this comment.
Test runs don't share workspaces (the jenkins dir where the repo gets cloned down, from which dockerized tests run, mapping in REPO_DIR), so there's no race.
There was a problem hiding this comment.
Great. That makes things easier.
| # We pass the host output dir as the source dir to `docker run -v`, but use | ||
| # the regular one to compute diff (they will be the same if running this | ||
| # test on the host, potentially different if running in a container). | ||
| REPO_DIR=${REPO_DIR:-"${KUBE_ROOT}"} |
There was a problem hiding this comment.
@jlowdermilk I moved the reference to REPO_DIR from the verify script to the update script.
caesarxuchao
force-pushed
the
dont-update-api-reference
branch
from
589f158 to
4e4c67c
Compare
|
@jlowdermilk, updated. It passed my local dockerized tests. PTAL. Thank you very much for your help! |
|
PR changed after LGTM, removing LGTM. |
k8s-github-robot
removed
the
lgtm
| HOST_OUTPUT_DIR="${REPO_DIR}/_tmp/api-reference" | ||
| TMP_OUTPUT_DIR="${KUBE_ROOT}/_tmp/api-reference" | ||
| HOST_OUTPUT_DIR="${KUBE_ROOT}/_tmp/api-reference" | ||
| mkdir -p ${HOST_OUTPUT_DIR} |
There was a problem hiding this comment.
I would recommend calling this something else, since it's not a host resolvable path. OUTPUT_DIR is fine.
|
GCE e2e test build/test passed for commit 589f158da9e96685c04c7b6d3bac565dbbf9f6ac. |
|
GCE e2e test build/test passed for commit 4e4c67c. |
|
GCE e2e test build/test passed for commit bd302d9. |
|
lgtm |
j3ffml
added
the
lgtm
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit bd302d9. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Fix #16121
cc @jlowdermilk because I changed the REPO_DIR.
cc @lavalamp