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 branching option to branch from another branch. #4531

Merged
merged 3 commits into from Jun 29, 2017

Conversation

Projects
None yet
3 participants
@adbridge
Contributor

adbridge commented Jun 12, 2017

Previously if updating a branch in the ARMmbed version of an example
repo, the branch would be created initially from master. This update
allows the new branch to be created from any pre-existing branch.
This update also moves the branch / fork / tag configuration data to the
json config file. It thus simplifies the command line.
-b on its own now indicates use the branch information in the config
-f on its own now indicates use the fork information in the config

Update branching option to branch from another branch.
Previously if updating a branch in the ARMmbed version of an example
repo, the branch would be created initially from master. This update
allows the new branch to be created by any pre-existing branch.
This update also moves the branch / fork / tag configuration data to the
json config file. It thus simplifies the command line.
-b on its own now indicates use the branch information in the config
-f on its own now indicates use the fork information in the config

@0xc0170 0xc0170 requested a review from theotherjimmy Jun 13, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 13, 2017

lol. love the title

@theotherjimmy

Looks fine. Nits, questions and future work below.

# Push new branch upstream
cmd = ['git', 'push', '-u', 'origin', dst]
return_code = run_cmd(cmd)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

If all of these commands share error behavior, could we have a construct like:

for cmd in [['git', 'checkout', src],
            ['git', 'checkout', '-b', dst],
            ['git', 'push', '-u', 'origin', dst]]:
    return_code = run_cmd(cmd)
    if return_code:
        break
else:
    update_log.error("Failed to prepare branch: %s", dst)

This comment has been minimized.

@adbridge

adbridge Jun 13, 2017

Contributor

Not sure that does what you think? If each command ran successfully then there would be no break , the loop would terminate and the 'else' clause run...
From python help:
"For loops also have an else clause which most of us are unfamiliar with. The else clause executes when the loop completes normally. This means that the loop did not encounter any break. ". Could be tweaked to replace the 'else' clause with an additional check of return_code though

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

Eh, fine use if return_code.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

Actually, I was thinking of:

for cmd in [['git', 'checkout', src],
            ['git', 'checkout', '-b', dst],
            ['git', 'push', '-u', 'origin', dst]]:
    return_code = run_cmd(cmd)
    if return_code:
        break
else:
    return True
update_log.error("Failed to prepare branch: %s", dst)
return False

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

OH, we can do better:

for cmd in [['git', 'checkout', src],
            ['git', 'checkout', '-b', dst],
            ['git', 'push', '-u', 'origin', dst]]:
    if run_cmd(cmd):
        update_log.error("Failed to prepare branch: %s", dst)
        return False
return True

This comment has been minimized.

@adbridge

adbridge Jun 15, 2017

Contributor

Have improved it further, this function doesn't really need to have it's own separate error so can actually just use run_cmd(cmd, exit_on_failure=True). It wasn't even checking the return from prepare_fork() or prepare_branch() anyway!

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 15, 2017

Contributor

Nice! so:

for cmd in [['git', 'checkout', src],
            ['git', 'checkout', '-b', dst],
            ['git', 'push', '-u', 'origin', dst]]:
    run_cmd(cmd, exit_on_failure=True)
body = "Please test this PR.\n"
body += "If successful then merge, otherwise provide a known issue.\n"
body += "Once you get notification of the release being made public then tag Master with " + tag

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

Could we make a note to replace all of these bodies with jinja templates? I think it would ease maintenance.

This comment has been minimized.

@adbridge

adbridge Jun 13, 2017

Contributor

Might be overkill for just the one case in this file ?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

Do you really think this is going to be the only case where we emit a blob of text? I think there may be more when we extend this script later.

exclusive.add_argument('-U', '--github_user', help="GitHub user for forked repos, mutually exclusive to branch option")
exclusive.add_argument('-b', '--branch', help="Branch to be updated, mutually exclusive to user option")
exclusive.add_argument('-f', '--fork', help="Update a fork", action='store_true')
exclusive.add_argument('-b', '--branch', help="Update a branch", action='store_true')

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

We could also do sumcommands here. Invocation would be python update.py fork vs python update.py -f

This comment has been minimized.

@adbridge

adbridge Jun 13, 2017

Contributor

Not sure i see what the benefit here is ?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 13, 2017

Contributor

in unix tradition, things starting with "-" are flags that are optional. before you use make.py, bulid.py or project.py as an example, remember that I did not write that bit, and it can't be changed.

This comment has been minimized.

@adbridge

adbridge Jun 14, 2017

Contributor

mmm but by that argument you would have something like :
update.py token 12eedf34455gggg
Which doesn't feel right to me at all -T 12eedf34455gggg is much more intuitive, but it is still a compulsory argument.. ?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 14, 2017

Contributor

Not quite. Do you need the github token to work correctly? Could you do a "dry run" without it?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 14, 2017

Contributor

@adbridge For things that are booleans/enums, you end up with subcommands, for things that are required and not booleans/enums, you end up with positional arguments. That's the unix tradition that I'm familiar with

This comment has been minimized.

@adbridge

adbridge Jun 14, 2017

Contributor

You also need the tag in there :)
I could live with having 'branch' or 'fork' as mutually exclusive sub-commands , think the others will stay as they are.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 14, 2017

Contributor

@adbridge This is really a style issue, so it comes down to your preference. I think that you will likely be the most common user of this script.

This comment has been minimized.

@adbridge

adbridge Jun 15, 2017

Contributor

Eventually the CI will be the user , but it would ultimately be nice to have a consistent approach across all our tools ...

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 15, 2017

Contributor

Well, computers aren't really the consumers. I'm expecting that you will write the script or job that calls this script in CI. That would make you the consumer.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 21, 2017

@theotherjimmy ok I think that's the last of the comments, can you just double check the last couple of commits? Thanks

@@ -475,13 +481,15 @@ def create_work_directory(path):
failures = []
successes = []
results = {}
template = dirname(abspath(__file__))

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 21, 2017

Contributor

I don't think this is the template. Maybe it's the template directory?

This comment has been minimized.

@adbridge

adbridge Jun 22, 2017

Contributor

yeah template directory, seemed like too long a name though ! Could have gone with tmpl_dir or similar I guess .

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

¯\_(ツ)_/¯

@theotherjimmy

Looks great!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

@adbridge Does this need to be run through CI?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 27, 2017

@theotherjimmy We can run through ci if we want but this script doesn't get tested by anything there...

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

Cool. Let's merge when Cam-CI gets back to us.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2017

retest uvisor

@theotherjimmy theotherjimmy merged commit 18b1cb6 into ARMmbed:master Jun 29, 2017

3 checks passed

Cam-CI uvisor Build & Test Success
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adbridge

This comment has been minimized.

Contributor

adbridge commented Jul 5, 2017

@theotherjimmy This was merged without a release label! Should really have gone to 5.5.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment