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

Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 #16681

Closed
wants to merge 2 commits into from
Closed

Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 #16681

wants to merge 2 commits into from

Conversation

aimeeu
Copy link
Contributor

@aimeeu aimeeu commented Oct 3, 2019

The current version of the script works with Python 2.7, PyYAML 3.13, Go 1.12.

This PR grew out of my attempts to run the website/update-imported-docs/update-imported-docs script using Python 3.7, Go 1.13, and PyYAML 5.1 -- Slack thread.Using Go 1.13 caused numerous GOPATH issues.

Neither the script nor documentation specify what version of Python the script was tested with, nor the version of Golang, Git, or PyYAML; docs page says Golang v 1.9 or later… yet Kubernetes development lists specific versions of Golang for specific versions of Kubernetes (see list). My issue was using Go 1.13, which Kubernetes does not support.

@tengqm agreed that the script should be updated to work with Python 3. I took this opportunity to "pythonify" the code.

Updating to the latest version of PyYAML required an update to remove a deprecated function. Looking at the code, I decided it would be better to use the Python tempfile library to create the temp directory rather than os and shutil libraries.

This script was tested on Ubuntu 18 with Python 3.7.4, go1.12.10 linux/amd64, and PyYAML 5.1.2 installed in a virtual environment.

Current version in master for comparison: https://github.com/kubernetes/website/blob/master/update-imported-docs/update-imported-docs

  • Add .py extension to update-imported-docs file
  • Update shebang for Python 3
  • Add script comments: software versions, what the script does, and how to run it
  • Update to use argparse.ArgumentParser() for command line args
  • Change work_dir from hard-coded dir to tmp dir created using the tempfile library; remove shutil.rmtree because it was throwing permission errors and also no longer needed after switching
    to temp dir created with tempfile.
  • Enhance error handling
  • Add check for Go installation
  • Enhance check for PyYAML installation
  • Change deprecated PyYAML load function to full_load https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation
  • Modify to comply with PEP8

Partial #15682

Also updated the software version in kubernetes-components.md #before-you-begin

Signed-off-by: Aimee Ukasick [email protected]

STEPS TO TEST THE SCRIPT
Scenario 1: PyYAML not installed.

  1. (Optional) Create a virtual environment with Python = 3.7.4
  2. cd to website/update-imported-script and execute the script $ ./update-imported-docs.py reference.yml - you should see a message that says Please ensure PyYAML package is installed (https://pypi.org/project/PyYAML/). This can be done, for example, by executing the following command: pip install PyYAML

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2019
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Oct 3, 2019
@netlify
Copy link

netlify bot commented Oct 3, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 6656021

https://deploy-preview-16681--kubernetes-io-master-staging.netlify.com

@aimeeu aimeeu changed the title [WIP] Update script for Python3, PyYAML 5.1 [WIP] Update website/update-imported-docs/update-imported-docs script for Python3, PyYAML 5.1 Oct 8, 2019
@aimeeu
Copy link
Contributor Author

aimeeu commented Oct 8, 2019

/unassign @sftim
/unassign @stewart-yu
/assign @tengqm
/assign @kbhawkey

@aimeeu
Copy link
Contributor Author

aimeeu commented Oct 8, 2019

/unassign @sftim
/unassign @stewart-yu

@aimeeu
Copy link
Contributor Author

aimeeu commented Oct 9, 2019

/uncc @sftim
/uncc @stewart-yu

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kbhawkey
You can assign the PR to them by writing /assign @kbhawkey in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aimeeu aimeeu changed the title [WIP] Update website/update-imported-docs/update-imported-docs script for Python3, PyYAML 5.1 Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 Oct 14, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2019
@aimeeu aimeeu changed the title Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 [WIP] Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 Oct 14, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2019
@kbhawkey
Copy link
Contributor

@aimeeu , do you need more feedback?

@aimeeu
Copy link
Contributor Author

aimeeu commented Oct 21, 2019

@kbhawkey - Thanks for the feedback so far. Updating got lost in the shuffle - plan to do that now.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. language/en Issues or PRs related to English language and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2019
-Update to use argparse.ArgumentParser() for command line args
-Use tempfile library to create tmp work directory

-Remove shutil.rmtree because it was throwing permission errors and
not deleting the directory. Also no longer needed after switching
to tempfile.

-Enchance error handling
-Add check for Go installation
-Enhance check for PyYAML installation
-Change deprecated PyYAML load function to full_load
-Add comments at top of file
-Modify to comply with PEP8
-Update kubernetes-components.md with software versions to match script

Signed-off-by: Aimee Ukasick <[email protected]>
@aimeeu aimeeu changed the title [WIP] Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 Update website/update-imported-docs/update-imported-docs script for Python 3, PyYAML 5.1.2 Oct 21, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 21, 2019
@@ -20,14 +20,15 @@ reference documentation for tools and components in the
* [Python 3.7.x](https://www.python.org/downloads/)
* [Git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git)
* [Golang](https://golang.org/doc/install) version 1.12 for Kubernetes 1.14+; Go 1.13 [is not supported](https://github.com/kubernetes/community/blob/master/contributors/devel/development.md#go)
* [Pip](https://pypi.org/project/pip/), which is used to install PyYAML (installed with Python)
* [Pip](https://pypi.org/project/pip/), which is used to install PyYAML (not installed by default on Ubuntu)
Copy link
Contributor

@kbhawkey kbhawkey Oct 22, 2019

Choose a reason for hiding this comment

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

@aimeeu, this is a nit: I would rather not see the comment about "... not installed ... on Ubuntu"). It seems clear just to say why Pip is a requirement.

Another nit,line 51: Is the $ needed:
$github.com/website. The remaining steps refer to your base directory as <web-base>.

Last nit 😃 : I may have brought this up before, but the Fixing Links section is confusing.
IIRC, this flag should be used to fix the release notes, not the generated markdown files.
Not sure if this script is even used to post process the release notes.

Copy link
Contributor Author

@aimeeu aimeeu Oct 22, 2019

Choose a reason for hiding this comment

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

@kbhawkey I agree. I removed that yesterday early afternoon. Hmmmmm.... it was after I rebased to remove a merge conflict. I have no idea why the doc change didn't get to pushed to the PR when I pushed.
My local copy:

    * [Python 3.7.x](https://www.python.org/downloads/)
    * [Git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git)
    * [Golang](https://golang.org/doc/install) version 1.12 for Kubernetes 1.14+; Go 1.13 [is not supported](https://github.com/kubernetes/community/blob/master/contributors/devel/development.md#go)
    * [Pip](https://pypi.org/project/pip/), which is used to install PyYAML
    * [PyYAML](https://pyyaml.org/) v5.1.2
    * [make](https://www.gnu.org/software/make/)
    * [gcc compiler/linker](https://gcc.gnu.org/)

and git status says "nothing to commit."

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow -- there are a lot of commits on the last commit. Did you rebase, something like, git fetch upstream,
git rebase -i upstream/master?

Copy link
Contributor Author

@aimeeu aimeeu Oct 22, 2019

Choose a reason for hiding this comment

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

Yes I did. Followed the instructions in the Intermediate contribute doc, and looked at the Community docs as well. Plus now there are two commits, which means I will have to squash, which means more rebasing. SIGH.

Copy link
Contributor

Choose a reason for hiding this comment

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

😬

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what happened, but the second commit seems to contain the latest change to that file?
At this pt, it may be worthwhile to get these changes in and correct the few remaining things in a second PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm abandoning this PR after I incorporate changes into a new one. I tried to revert the merge commit, which ended in total disaster.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not merge this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not able to squash the commits. However, you are an approver so if you want to merge it, that's fine. Otherwise, I have a new PR ready to go. Let me know what you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since I have no idea what happened, I suppose you should create another PR.
Based upon what I see in the preview and the source, the changes seem fine.

@aimeeu
Copy link
Contributor Author

aimeeu commented Oct 22, 2019

irreparable rebase issues; opened new PR #17127
/close

@k8s-ci-robot
Copy link
Contributor

@aimeeu: Closed this PR.

In response to this:

irreparable rebase issues; opened new PR #17127
/close

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.

@aimeeu aimeeu deleted the aimeeu-refactorUpdateImportedDocsScript branch November 18, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants