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

Swagger generated APIs from master #10336

Merged
merged 1 commit into from Feb 26, 2020

Conversation

Ropes
Copy link
Contributor

@Ropes Ropes commented Feb 25, 2020

Obviously there have been some changes to code and swagger declarations
which weren't built.

Commands run: make generate-api & make generate-health-api & git add api*. That's it.

Backstory

These changes were noticed when making a modification to the server.gotmpl file for #10116. Thus creating a requirement to separate API master code, from what was being changed for the syscall package removal.

@aanm asked me to build all of the swagger generated code from master so the diff between #10158 was limited to only the syscall->unix package changes.

Related: #10116

Signed-off-by: Joshua Roppo joshroppo@gmail.com


This change is Reviewable

@Ropes Ropes requested a review from a team as a code owner February 25, 2020 18:20
@maintainer-s-little-helper
Copy link

Commit c4b8d0c866522970ad96bde6b430ab8f866e01d0 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/contributing/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 25, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

1 similar comment
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 25, 2020
@Ropes Ropes force-pushed the pr/ropes/swagger/build/master branch from c4b8d0c to a81d1ea Compare February 25, 2020 18:22
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 25, 2020
@Ropes Ropes force-pushed the pr/ropes/swagger/build/master branch 2 times, most recently from 8aaa177 to f411adc Compare February 25, 2020 18:41
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for preparing this! At first glance this seems good, just one comment for the last couple of files below.

api/v1/health/server/server.go Outdated Show resolved Hide resolved
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Feb 25, 2020
@Ropes Ropes force-pushed the pr/ropes/swagger/build/master branch from f411adc to d6a11c3 Compare February 25, 2020 19:10
@Ropes
Copy link
Contributor Author

Ropes commented Feb 25, 2020

Alright.... make build passing(at least).

Obviously there have been some changes to code and swagger declarations
which weren't built.

* Commands run: `make generate-api` & `make generate-health-api`.
* Then removed `api/v1/health/models/health_response.go` on
request of @aanm.
* Unstaged swagger removed `// +k8s:deepcopy-gen=true` lines on
request of @joestringer.
* Removed api/v1/health/server/server.go * api/v1/server/server.go
due to build failure, and odd import changes:
https://github.com/cilium/cilium/pull/10336/files/f411adcbf09a9cf827438a59c189bc8cbe62f54c#r384055153

Related: cilium#10116

Signed-off-by: Joshua Roppo joshroppo@gmail.com
@Ropes Ropes force-pushed the pr/ropes/swagger/build/master branch from d6a11c3 to d622306 Compare February 25, 2020 19:17
@coveralls
Copy link

coveralls commented Feb 25, 2020

Coverage Status

Coverage decreased (-0.02%) to 45.476% when pulling d622306 on Ropes:pr/ropes/swagger/build/master into 3b055d1 on cilium:master.

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

What can we do to not miss these ones? 🤔

@aanm
Copy link
Member

aanm commented Feb 25, 2020

test-me-please

@joestringer
Copy link
Member

joestringer commented Feb 25, 2020

test-me-please

EDIT: Woops, github was out of sync so I accidentally re-triggered.

@Ropes
Copy link
Contributor Author

Ropes commented Feb 25, 2020

@aanm the failure of swagger to produce breaking changes could be caught by running the swagger generation, and then running make build. Maybe generate-k8s-api too? The underlying cause of the import issue(which still exists) would need to be immediately fixed in that case.

Running the swagger generation on every PR would expose the issues which my first PR uncovered. I'll re-list the issues uncovered in this yak shaving exercise:

  • Swagger incorrect generation of api/v1/health/models/health_response.go (Sounds like this issue is already being tracked)
  • swagger removes // +k8s:deepcopy-gen=true which breaks downstream consumer.(make generate-k8s-api IIUC)
  • Swagger injects invalid/breaking import statements into: api/v1/health/server/server.go & api/v1/server/server.go Cause unknown.

Those are all the issues I know of at this point.

@joestringer joestringer merged commit c2939fe into cilium:master Feb 26, 2020
1.8.0 automation moved this from In progress to Merged Feb 26, 2020
@Ropes Ropes deleted the pr/ropes/swagger/build/master branch February 26, 2020 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants