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 docs on GitHub integration, mention --new-pr/--update-pr #199

Merged
merged 11 commits into from
Jun 7, 2016

Conversation

boegel
Copy link
Member

@boegel boegel commented Feb 13, 2016

@boegel boegel added this to the v2.7.0 milestone Feb 17, 2016
@boegel boegel modified the milestones: v2.7.0, v2.8.0 May 2, 2016
@boegel boegel changed the title update docs on GitHub integration, mention --new-pr/--update-pr (WIP) update docs on GitHub integration, mention --new-pr/--update-pr Jun 3, 2016
@boegel boegel modified the milestones: v2.9.0, v2.8.0 Jun 3, 2016
@boegel
Copy link
Member Author

boegel commented Jun 3, 2016

@wpoely86, @pescobar, @fgeorgatos, @rjeschmi I finally got around to completing this... Please review?

@pescobar
Copy link
Member

pescobar commented Jun 3, 2016

why not enable --test-report-env-filter by default and enable it only in case it's needed and you know what you are doing?

maybe it's worth mentioning that a recent python-keyring version is needed? I remember I had issues with this but I don't rember the specific version I had to use...

otherwise, lgtm

@boegel
Copy link
Member Author

boegel commented Jun 3, 2016

@pescobar well, I guess we could change the default to the filter being mentioned in the documentation now, sure... @geimer: thoughts on this, since you cooked this up?

@besserox
Copy link

besserox commented Jun 4, 2016

@boegel Quick feedback about the --git-working-dirs-path option:

It says location of your Git working directories, which one? easyconfig, easyblock, ... ? only one? multiples? all of them?

I think this could be clarified.


This takes care of all the steps required to make a contribution, i.e.:

* set up a working copy of the relevant EasyBuild repository (e.g., ``easybuild-easyconfigs``)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit unclear to me. From the example, it seems that only easyconfigs can be handles by this option. But with the information about --pr-target-repo below, I get the impression that also framework/easyblock PRs can be handled. However, in this case, the question is how the automatic renaming/moving works.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geimer For now, in the experimental stage, --new-pr and --update-pr only support easyconfigs, but the intention is to extend this to easyblocks and framework too.

For easyblocks, we can derive the correct name & location as well. For framework, we will only be able to support patching of existing modules, but that should cover 95% of all uses.

The documentation is written with that in mind, and so is the --pr-target-repo configure option.

@geimer
Copy link
Contributor

geimer commented Jun 6, 2016

@boegel: I don't think I was involved in this stuff... But anyway, I did a quick review and in general it looks reasonable to me.

@boegel
Copy link
Member Author

boegel commented Jun 6, 2016

@geimer I was pinging you w.r.t. the value for --test-report-env-filter in particular, I think you came up with the filter that is mentioned in the documentation?

@pescobar's suggestion was to make that the default...

@geimer
Copy link
Contributor

geimer commented Jun 6, 2016

@boegel: I don't think so. A --test-report-env-filter example was already in the wiki when I started using EasyBuild...

@boegel
Copy link
Member Author

boegel commented Jun 6, 2016

@geimer hmm, ok, sorry for the noise then, I must be misremembering...

@boegel
Copy link
Member Author

boegel commented Jun 7, 2016

going in, thanks for the feedback everyone!

@boegel boegel merged commit d73f315 into easybuilders:develop Jun 7, 2016
@boegel boegel deleted the new_update_pr branch June 7, 2016 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants