8000 go-swagger: fix panic under Golang 1.13 by kolyshkin · Pull Request #40038 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

go-swagger: fix panic under Golang 1.13 #40038

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2019
Merged

go-swagger: fix panic under Golang 1.13 #40038

merged 1 commit into from
Oct 10, 2019
+4 −2

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin mentioned this pull request Oct 4, 2019
Merged
thaJeztah
thaJeztah reviewed Oct 4, 2019
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

You'll have to add a commit that changes the swagger.yaml, otherwise go-swagger won't run

@thaJeztah
Copy link
Member

Failing, looks like you're referencing a wrong commit;

error: pathspec '5793aa66d4b4112c2602c716516e24710e4adbb5 # golang-1.13-fix in kolyshkin/go-swagger' did not match any file(s) known to git.

@kolyshkin
Copy link
Contributor Author

error: pathspec '5793aa66d4b4112c2602c716516e24710e4adbb5 # golang-1.13-fix in kolyshkin/go-swagger' did not match any file(s) known to git.

It's just I added a comment to an inappropriate place. Removed.

@thaJeztah
Copy link
Member

ah, lol you just pushed again

@kolyshkin
Copy link
Contributor Author

OK looks like it works:

[2019-10-04T23:46:23.571Z] The swagger spec at "api/swagger.yaml" is valid against swagger specification 2.0

Now I'm not sure what would be the best way to have it. Temporary, we can merge this one as-is (removing the second commit of course). Longer-term, IDK.

@thaJeztah thaJeztah mentioned this pull request Oct 7, 2019
Merged
@thaJeztah
10000 Copy link
Member

Now I'm not sure what would be the best way to have it. Temporary, we can merge this one as-is (removing the second commit of course). Longer-term, IDK.

Some thoughts;

  • we don't (currently) need the new features of newer go-swagger versions, so it should be ok to keep using this old version for now
  • this tool is only used locally, so if future Go updates break it again, perhaps we could consider pinning it to a fixed version of go (and/or pre-building the binaries)

thaJeztah
thaJeztah reviewed Oct 8, 2019
View reviewed changes
Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

If we decide to merge this; perhaps we should tag the commit (also wondering if the fork should live in the moby org, so that it's not tied to a personal account)

@kolyshkin
Copy link
Contributor Author

OK here's my plan

  1. merge this one as a temp fix to unblock CI
  2. Try making upstream go-swagger work for us
  3. If the above fails (I don't want to spend too much time on it), move the fork under moby and use it.

@kolyshkin kolyshkin marked this pull request as ready for review October 8, 2019 18:21
thaJeztah
thaJeztah reviewed Oct 8, 2019
View reviewed changes
Dockerfile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a TODO comment, and open a tracking issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kolyshkin
go-swagger: fix panic
93f9b90 
This is an attempt to fix go-swagger panic under Golang 1.13.

Details:
 * go-openapi/jsonpointer#4
 * go-swagger/go-swagger#2059

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Rebased, added TODO

thaJeztah
thaJeztah approved these changes Oct 8, 2019
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah changed the title [WIP] go-swagger: fix panic go-swagger: fix panic Oct 8, 2019
@thaJeztah
Copy link
Member

Weird; CI Hung after tests completed; https://ci.docker.com/public/job/moby/job/PR-40038/6/execution/node/195/log/?consoleFull

00:55:32.976  DONE 270 tests, 6 skipped in 2585.326s

...


02:00:04.686  Sending interrupt signal to process
02:00:04.715  Sending interrupt signal to process
02:00:13.230  /home/ubuntu/workspace/moby_PR-40038@tmp/durable-89316c98/script.sh: line 5:  9079 Terminated              docker run $rm -t --privileged -v "$WORKSPACE/bundles/${TEST_INTEGRATION_DEST}:/go/src/github.com/docker/docker/bundles" -v "$WORKSPACE/bundles/dynbinary-daemon:/go/src/github.com/docker/docker/bundles/dynbinary-daemon" -v "$WORKSPACE/.git:/go/src/github.com/docker/docker/.git" --name "$CONTAINER_NAME" -e KEEPBUNDLE=1 -e TESTDEBUG -e TESTFLAGS -e TEST_SKIP_INTEGRATION -e TEST_SKIP_INTEGRATION_CLI -e DOCKER_GITCOMMIT=${GIT_COMMIT} -e DOCKER_GRAPHDRIVER -e TIMEOUT -e VALIDATE_REPO=${GIT_URL} -e VALIDATE_BRANCH=${CHANGE_TARGET} docker:${GIT_COMMIT} hack/make.sh "$1" test-integration
02:00:13.230  Remaining pids to kill: []
02:00:13.235  script returned exit code 143

@thaJeztah thaJeztah added the kind/bugfix PR's that fix bugs label Oct 10, 2019
@tiborvass tiborvass merged commit b4e912b into moby:master Oct 10, 2019
@thaJeztah thaJeztah mentioned this pull request Oct 14, 2019
Merged
6 tasks
@thaJeztah thaJeztah changed the title go-swagger: fix panic go-swagger: fix panic under Golang 1.13 Jan 17, 2020
@thaJeztah thaJeztah mentioned this pull request Feb 22, 2020
Merged
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiborvass tiborvass tiborvass approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
area/testing kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review
Projects
None yet
Milestone
20.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kolyshkin @thaJeztah @tiborvass
0