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.

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

5057 restartpolicy default #8592

Merged
merged 1 commit into from May 27, 2015
Merged

5057 restartpolicy default #8592

merged 1 commit into from May 27, 2015

Conversation

RichieEscarez
Copy link
Contributor

  • Updated two help topics to clarify that the default = always
  • Updated all the types.go files to include statement of what default is (in descriptions)
  • Ran "hack/update-swagger-spec.sh" to build output .json files for swagger UI
    (note, it looks like the script was not run in a long while and this output includes several other changes that i did not deliver)

Fixes #5057

@RichieEscarez RichieEscarez added the kind/documentation Categorizes issue or PR as related to documentation. label May 20, 2015
@davidopp
Copy link
Member

Great, thanks! Just a few comments:

  1. I noticed that update-swagger-spec.sh was never updated to generate
    ${SWAGGER_ROOT_DIR}/v1.json
    Can you add to that script a line at the end that does
    curl ${SWAGGER_API_PATH}api/v1 > ${SWAGGER_ROOT_DIR}/v1.json
    and then re-run the script and add the new file it generates (v1.json) to your PR?

  2. Our standard practice on the team is to squash all the commits into one, so please squash your two commits into one

  3. After squashing (or as part of squashing), please add to the commit message "Fixes Document default restart policy in description tag #5057" so that merging automatically closes the issue.

@RichieEscarez RichieEscarez force-pushed the 5057_restartpolicy_default branch 2 times, most recently from e5551ca to 4e7a9e3 Compare May 21, 2015 15:18
@RichieEscarez
Copy link
Contributor Author

Thanks David for all the help on this.

I asked Nikhil because i was concerned and he suggested that I leave the .json output as a separate commit since the build i ran picked up a bunch of other changes that werent mine (ill remember to comment on my intentions next time).

Also to share what i learned (and for future reference), you need the following, in this order, to build the new .json files:

  1. Environment setup as defined here (where GO and Etcd are installed and in your PATH and k8s is cloned to your GOPATH)
  2. Run "hack/build-go-sh"
  3. Run "hack/update-swagger-spec.sh"

@k8s-bot
Copy link

k8s-bot commented May 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

davidopp
davidopp reviewed May 22, 2015
View reviewed changes
api/swagger-spec/v1.json
"apiVersion": "",
"basePath": "",
"resourcePath": ""
}
Copy link
Member

Choose a reason for hiding this comment

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

Please insert newline at end of file.

@davidopp
Copy link
Member

Hmm, the v1.json file that got generated is empty. That's weird. Maybe just remove your update-swagger-spec.sh changes, and v1.json, from this PR, and then we could fix update-swagger-spec.sh and add v1.json in a followup PR. Sorry for sending you down that road; I'm not sure what the problem is.

@RichieEscarez RichieEscarez force-pushed the 5057_restartpolicy_default branch 2 times, most recently from 10f2014 to 982538e Compare May 22, 2015 16:59
@RichieEscarez
Edited to add the default restart policy (always) to clarify value if…
0076c18 
… RestartPolicy is not set.

Generated new .json output for Swagger UI

Added the api/v1/v1.json file to the hack/update-swagger-spec.sh script so it gets updated and built.

Fixes kubernetes#5057

Reverting last change: Deleted line for building v1 api from the  build file update-swagger-ui.sh and deleted the output file from build (v1.json).
@davidopp
Copy link
Member

LGTM

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2015
thockin added a commit that referenced this pull request May 27, 2015
@thockin
Merge pull request #8592 from RichieEscarez/5057_restartpolicy_default
b8a808b 
5057 restartpolicy default
@thockin thockin merged commit b8a808b into kubernetes:master May 27, 2015
@RichieEscarez RichieEscarez deleted the 5057_restartpolicy_default branch May 27, 2015 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidopp davidopp

Labels
kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document default restart policy in description tag
5 participants
@RichieEscarez @davidopp @k8s-bot @googlebot @thockin