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

🌱 Fix yaml file write issue in create-local-repository.py #3648

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

Jaakko-Os
Copy link
Contributor

@Jaakko-Os Jaakko-Os commented Sep 16, 2020

What this PR does / why we need it:
This PR fixes a file write issue in create-local-repository.py which is detected during component upgrade using clusterctl.

Which issue(s) this PR fixes

Traceback (most recent call last):
File "cmd/clusterctl/hack/create-local-repository.py", line 130, in write_local_repository
f.write(components_yaml)
TypeError: write() argument must be str, not bytes

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "cmd/clusterctl/hack/create-local-repository.py", line 241, in
repos = list(create_local_repositories())
File "cmd/clusterctl/hack/create-local-repository.py", line 162, in create_local_repositories
components_path = write_local_repository(provider, next_version, components_file, components_yaml)
File "cmd/clusterctl/hack/create-local-repository.py", line 138, in write_local_repository
raise Exception('failed to write {} to {}: {}'.format(components_file, provider_folder, e))
Exception: failed to write core-components.yaml to /home/****/.cluster-api/dev-repository/cluster-api/v0.3.8: write() argument must be str, not bytes

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 16, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @Jaakko-Os. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 16, 2020
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 16, 2020
@Jaakko-Os
Copy link
Contributor Author

/cc @bboreham

@k8s-ci-robot
Copy link
Contributor

@Jaakko-Os: GitHub didn't allow me to request PR reviews from the following users: bboreham.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @bboreham

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.

@fabriziopandini
Copy link
Member

Given that this reverts #3614, I would like to get @bboreham feedback here
Both versions are working fine for me...

@fabriziopandini
Copy link
Member

/area clusterctl
/milestone v0.3.10

@k8s-ci-robot k8s-ci-robot added this to the v0.3.10 milestone Sep 16, 2020
@k8s-ci-robot k8s-ci-robot added the area/clusterctl Issues or PRs related to clusterctl label Sep 16, 2020
@fabriziopandini
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2020
@Jaakko-Os
Copy link
Contributor Author

Given that this reverts #3614, I would like to get @bboreham feedback here
Both versions are working fine for me...

Please note that this PR reverts https://github.com/kubernetes-sigs/cluster-api/blob/master/cmd/clusterctl/hack/create-local-repository.py#L129

but leaves https://github.com/kubernetes-sigs/cluster-api/blob/master/cmd/clusterctl/hack/create-local-repository.py#L181 unchanged

@bboreham
Copy link
Contributor

Like I said, we should get someone who knows Python to comment.

@vincepri
Copy link
Member

Wild thought 😄 , could we rewrite this simple python program in Go?

@Jaakko-Os
Copy link
Contributor Author

$ python --version
Python 3.6.8

@Jaakko-Os Jaakko-Os changed the title 🐛 Fix yaml file write issue Fix yaml file write issue Sep 17, 2020
@Jaakko-Os
Copy link
Contributor Author

Wild thought smile , could we rewrite this simple python program in Go?

@vincepri @bboreham @fabriziopandini could we get this issue forward to reach a solution?
I'm running controlplane component upgrade using clusterctl and this is a stopping issue for me.
I would consider the possible rewriting of 'create-local-repository.py' a sepate task.

@fabriziopandini
Copy link
Member

I will try to give another run locally, but we should seek for a solution that works for both.

@fabriziopandini
Copy link
Member

With the change in this PR the script works both on python 2.7.16 and python 3.8.0.
@bboreham could you kindly confirm the change works also for you?

@Jaakko-Os Jaakko-Os changed the title Fix yaml file write issue Fix yaml file write issue in create-local-repository.py Sep 21, 2020
Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 21, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged

Fixes: metal3-io#427
@vincepri
Copy link
Member

@Jaakko-Os Can you fix the commit so it doesn't include or reference a github issue or PR?

@Jaakko-Os
Copy link
Contributor Author

Jaakko-Os commented Sep 22, 2020

@Jaakko-Os Can you fix the commit so it doesn't include or reference a github issue or PR?

done

Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 22, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged

Fixes: metal3-io#427
Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 22, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged

Fixes: metal3-io#427
Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 22, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged

Fixes: metal3-io#427
Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 22, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged

Fixes: metal3-io#427
File "cmd/clusterctl/hack/create-local-repository.py", line 130, in write_local_repository
f.write(components_yaml)
TypeError: write() argument must be str, not bytes

kubernetes-sigs#3647
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 22, 2020
@Jaakko-Os
Copy link
Contributor Author

/retest

Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 22, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade

Workarounds
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged
-added wait for the controllers loop due to environment fluctuation

Fixes: metal3-io#427
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/retitle 🌱 Fix yaml file write issue in create-local-repository.py
/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot changed the title Fix yaml file write issue in create-local-repository.py 🌱 Fix yaml file write issue in create-local-repository.py Sep 22, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020
@fabriziopandini
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot merged commit 4562836 into kubernetes-sigs:master Sep 22, 2020
Jaakko-Os added a commit to Nordix/metal3-dev-env that referenced this pull request Sep 23, 2020
The combined upgrade consists of
-controlplane component upgrade using clusterctl
-k8s version and disk image upgrade for
 master and worker
-ironic image upgrade

Workarounds
-local-overrides workaround included until
 kubernetes-sigs/cluster-api#3648 is merged
-added wait for the controllers loop due to environment fluctuation

Fixes: metal3-io#427
@Jaakko-Os Jaakko-Os deleted the fix-write-local-repo branch September 23, 2020 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants