add tool for pushing vendor updates to consuming repos easier #687

dhellmann · 2020-08-25T21:00:39Z

No description provided.

dhellmann · 2020-08-25T21:06:18Z

hack/push_updates.sh

Danil-Grigorev · 2020-08-26T14:15:04Z

hack/push_updates.sh

+if [ -z "$SHA" ]; then
+    echo "Determining SHA for machine-api-operator repository..."
+    hub clone openshift/machine-api-operator
+    (cd ./machine-api-operator && git checkout origin/$BRANCH_NAME)


I wonder if this should not be done in a temporary directory, and additionally cleaned up after completion?

It is a temporary directory, see lines 86-87.

I didn't add a trap to clean it up because @elmiko wanted it to run in a container, and I figured everything would be removed when the container is removed. Maybe that assumption isn't safe or some folks will run this outside of the container?

i certainly don't want to see us do extra work just for the container option. that said, it should be relatively safe to leave artifacts in a temporary directory as it will get cleaned by the o/s as needed. additionally it might help for debugging to have the artifacts.

It certainly helped while developing the script. :-)

Danil-Grigorev · 2020-08-26T14:40:45Z

hack/push_updates.sh

+# Push updates that need to be vendored in other repositories.
+
+if ! which hub 2>/dev/null 1>&2; then
+    echo "This tool requires the hub command line app." 1>&2


Is it possible just to use git tool?.. Don't know all the details, but from visual inspection, it is possible to replicate all commandls with git clone and git remote add ...

git can't create the fork if you don't already have it

elmiko

this looks really nice to me, thanks Doug!

definitely think we should get Joel's thoughts, but he is out till next week.

/lgtm

dhellmann · 2020-08-28T20:34:53Z

/retest

JoelSpeed

Looks good. My only concern is about the SHA length causing long commit messages and long branch names, should we always truncate to a 7 digit short commit SHA to prevent these long messages? Should still work with short SHAs right?

JoelSpeed · 2020-09-03T12:31:10Z

hack/push_updates.sh

+  -s SHA
+    The SHA from machine-api-operator repository to push.
+    Defaults to HEAD of master.


Does this need a full SHA or will it work with the 7 character short SHA? Might be worth a note in the help if it supports one and not the other

It's actually any reference, but it seemed safer to suggest using a SHA because if you just pass "HEAD" or a branch name then the commit message wouldn't be very meaningful. The short SHA should be OK.

JoelSpeed · 2020-09-03T13:00:53Z

hack/push_updates.sh

+    hub clone openshift/$repo
+    pushd ./$repo
+
+    hub fork --remote-name $REMOTE_NAME


Does this succeed if there is already a fork?

Yes, it recognizes the fork even if it has a different name and sets up the remote appropriately.

hack/push_updates.sh

JoelSpeed · 2020-09-03T15:52:32Z

I think I'm happy with this as is now, would be good to see it in action (maybe dry run?) so we can verify it works before merging, what do people think?

dhellmann · 2020-09-03T17:39:16Z

I think I'm happy with this as is now, would be good to see it in action (maybe dry run?) so we can verify it works before merging, what do people think?

Sure, that makes sense. Do you want me to do that and link to some PRs, or do you want to give it a spin to see how it behaves first hand?

/retest

JoelSpeed · 2020-09-04T10:37:48Z

Just cloned this myself to have a play, not sure how important these are but some observations:

You can't get the help text without hub installed
The name of the script uses an underscore, our convention for this is hyphens (all bar 1 other are hyphens)
Using dry run, I got an error Error creating fork: JoelSpeed/cluster-api-provider-azure already exists on github.com which I wasn't expecting to see, this caused the script to exit. My CAPZ fork is directly from kubernetes-sigs, not from the openshift fork, I wonder how many people will have that problem or if there is a workaround 🤔
Commit message was Update machine-api-operator to 8f5ef9 for AWS, looks good, had intended to have 7 chars but my suggestion should have been ${SHA:0:7} instead for 7 characters, not sure how many we want?
The AWS clone, update commit all seemed to work well :)

dhellmann · 2020-09-04T21:00:56Z

Just cloned this myself to have a play, not sure how important these are but some observations:

You can't get the help text without hub installed

Fixed

The name of the script uses an underscore, our convention for this is hyphens (all bar 1 other are hyphens)

Fixed

Using dry run, I got an error Error creating fork: JoelSpeed/cluster-api-provider-azure already exists on github.com which I wasn't expecting to see, this caused the script to exit. My CAPZ fork is directly from kubernetes-sigs, not from the openshift fork, I wonder how many people will have that problem or if there is a workaround 🤔

I changed the logic so the fork isn't created in dry-run mode.

I think if you create a fork yourself using a different repository name the hub command will use the Github API to figure that out. I could also see if I can make it try a simple clone if the fork fails.

Commit message was Update machine-api-operator to 8f5ef9 for AWS, looks good, had intended to have 7 chars but my suggestion should have been ${SHA:0:7} instead for 7 characters, not sure how many we want?

I switched to using the abbreviated hash computed by git instead of truncating it in the script.

The AWS clone, update commit all seemed to work well :)

dhellmann · 2020-09-04T21:08:46Z

I think if you create a fork yourself using a different repository name the hub command will use the Github API to figure that out. I could also see if I can make it try a simple clone if the fork fails.

On second thought, I'm not sure that's actually possible.

I wonder if because of the error handling we would actually want the script to run in 2 different phases. First to build all of the patches, and then either a second run to submit them or just leave it up to the user to submit them (and automate submission separately when we put this into a prow job).

Thoughts?

JoelSpeed

I switched to using the abbreviated hash computed by git instead of truncating it in the script.

+1 for this

I think the fallback to simple clone looks good, I haven't tested it, but I think it should work. I personally would prefer to keep the script so that it automates the whole thing. If I have to delete some of my forks and have them properly forked from the openshift forks to make that work, I think that's a sacrifice I'd be willing to make.

JoelSpeed · 2020-09-07T09:10:49Z

hack/push-updates.sh

+
+    # Use -d option to avoid build errors because we aren't going to
+    # build locally.
+    go get -d github.com/openshift/machine-api-operator@${FULL_SHA}


I don't think FULL_SHA is initialised when SHA is set explicitly, looks like it's only set when SHA is empty and we work it out in the script?

I fixed that, and also split the operation into 2 loops as we discussed in the meeting this morning.

JoelSpeed · 2020-09-11T14:03:22Z

@dhellmann Used the script earlier and it pretty much worked a treat! (https://bugzilla.redhat.com/show_bug.cgi?id=1877743) Had to manually fix one up because they're using a replace for one of the dependencies, and I had to set github.user in my git config, but then the fallback to simple clone worked as well.

Only thing I will say is that it doens't look like ovirt is in a place where they need this script just yet, might be worth dropping them from the defaults for now.

From this experience, future improvement ideas:

Reuse an existing directory: In case of failure, could re-use existing cloned resources?
Allow a PR description to be provided
Pause on commit failure to allow you to fix up, then continue?

dhellmann · 2020-09-11T21:34:17Z

@dhellmann Used the script earlier and it pretty much worked a treat! (https://bugzilla.redhat.com/show_bug.cgi?id=1877743) Had to manually fix one up because they're using a replace for one of the dependencies, and I had to set github.user in my git config, but then the fallback to simple clone worked as well.

I think I assumed the GitHub.user setting was part of configuring hub, but apparently not. It looks like the script could read ~/.config/hub instead. At the very least we could have it require the user to set up the git config it's expecting.

Only thing I will say is that it doens't look like ovirt is in a place where they need this script just yet, might be worth dropping them from the defaults for now.

Makes sense.

From this experience, future improvement ideas:

Reuse an existing directory: In case of failure, could re-use existing cloned resources?

I thought about making it create the working directory with the SHA in the name, so it could find it again later. Then it could skip building the patch if it sees a cloned repo in that predictable temp directory, assuming that the user would have fixed whatever caused the failure before running again. What do you think?

Allow a PR description to be provided

Definitely.

Pause on commit failure to allow you to fix up, then continue?

See above

dhellmann · 2020-09-11T21:36:31Z

I rebased, squashed, and removed ovirt from the list of repositories.

dhellmann · 2020-09-15T12:52:59Z

/retest

JoelSpeed · 2020-09-21T13:43:59Z

/approve

Thanks @dhellmann

openshift-ci-robot · 2020-09-21T13:44:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

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

elmiko

looks mostly good to me, just had one question

elmiko · 2020-09-21T14:16:06Z

hack/push-updates.sh

+
+function fork_repo () {
+    local repo_name="$1"
+    local remote_name="$2"


it seems like remote_name is unused?

d'oh, it's using REMOTE_NAME below instead of the local variable.

i figured it had to be one or the other XD

I removed the extra variable.

elmiko

/lgtm

openshift-bot · 2020-09-22T00:03:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T01:34:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T01:47:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T05:02:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T06:46:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T09:35:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T09:48:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T11:06:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T11:19:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T12:49:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T13:02:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T14:07:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-22T14:46:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

JoelSpeed · 2020-09-22T15:05:12Z

/hold

This will just keep retesting unnecessarily until it's merged

Do we want to get this merged before master opens or do we not really care?

dhellmann · 2020-09-22T15:07:39Z

It doesn't really make any difference to me, so if you'd like to wait that's fine.

JoelSpeed · 2020-09-22T15:33:14Z

Saves us having to create a BZ, so let's just hold cancel when master opens, we can all use it from here until then

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

elmiko · 2020-09-22T16:33:14Z

just wanted to note that i tried the script out in dry run mode today and everything is working as i would expect.

JoelSpeed · 2020-10-05T16:02:43Z

/hold cancel

I think this should be able to merge now that the 4.6 branch has been cut 🤔

elmiko · 2020-10-05T16:06:01Z

/lgtm

openshift-bot · 2020-10-06T01:08:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-06T01:34:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-06T05:04:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-10-06T06:48:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from alexander-demicev and Danil-Grigorev August 25, 2020 21:00

openshift-ci-robot requested review from elmiko and JoelSpeed August 25, 2020 21:06

Danil-Grigorev reviewed Aug 26, 2020

View reviewed changes

dhellmann force-pushed the push-update-tool branch from d07286e to f07e150 Compare August 26, 2020 17:34

elmiko approved these changes Aug 26, 2020

View reviewed changes

openshift-ci-robot assigned elmiko Aug 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020

JoelSpeed reviewed Sep 3, 2020

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2020

JoelSpeed reviewed Sep 7, 2020

View reviewed changes

dhellmann force-pushed the push-update-tool branch from 617aadf to fa139d1 Compare September 11, 2020 21:36

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2020

elmiko reviewed Sep 21, 2020

View reviewed changes

dhellmann force-pushed the push-update-tool branch from fa139d1 to 5d7fbe4 Compare September 21, 2020 14:22

elmiko approved these changes Sep 21, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020

add tool for pushing vendor updates to consuming repos easier

fc39af7

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the push-update-tool branch from 5d7fbe4 to fc39af7 Compare September 22, 2020 16:01

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2020

openshift-merge-robot merged commit d3599f4 into openshift:master Oct 6, 2020

add tool for pushing vendor updates to consuming repos easier #687

add tool for pushing vendor updates to consuming repos easier #687

Conversation

dhellmann commented Aug 25, 2020

dhellmann commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

dhellmann commented Aug 28, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Sep 3, 2020

dhellmann commented Sep 3, 2020

JoelSpeed commented Sep 4, 2020

dhellmann commented Sep 4, 2020

dhellmann commented Sep 4, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Sep 11, 2020

dhellmann commented Sep 11, 2020

dhellmann commented Sep 11, 2020

dhellmann commented Sep 15, 2020

JoelSpeed commented Sep 21, 2020

openshift-ci-robot commented Sep 21, 2020

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

openshift-bot commented Sep 22, 2020

JoelSpeed commented Sep 22, 2020

dhellmann commented Sep 22, 2020

JoelSpeed commented Sep 22, 2020

elmiko commented Sep 22, 2020

JoelSpeed commented Oct 5, 2020

elmiko commented Oct 5, 2020

openshift-bot commented Oct 6, 2020

openshift-bot commented Oct 6, 2020

openshift-bot commented Oct 6, 2020

openshift-bot commented Oct 6, 2020