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.

Sign up for GitHub

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

Jump to bottom

[ADD] extra arguments for 3 commands oca-gen-addons-table, oca-gen-addon-readme, oca-gen-addon-icon #103

Merged
merged 1 commit into from
Mar 28, 2020
Merged

[ADD] extra arguments for 3 commands oca-gen-addons-table, oca-gen-addon-readme, oca-gen-addon-icon #103

merged 1 commit into from
Mar 28, 2020

Conversation

legalsylvain
Copy link
Collaborator

WIP.

Add configuration options in environment file

sbidoul
sbidoul reviewed Mar 23, 2020
View reviewed changes
src/oca_github_bot/process.py Outdated
def check_call(cmd, cwd, log_error=True):
def check_call(cmd, cwd, log_error=True, extra_cmd=False):
if extra_cmd:
cmd += extra_cmd.split(",")
Copy link
Member

Choose a reason for hiding this comment

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

I would put this split in configuration parsing. So here assume extra_cmd is a list.

Copy link
Member

Choose a reason for hiding this comment

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

call this extra_cmd_args?

Copy link
Member

Choose a reason for hiding this comment

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

splitting on whitespace is probably more natural (and we can consider arguments containing spaces are not supported at the moment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would put this split in configuration parsing. So here assume extra_cmd is a list.

Indeed.

call this extra_cmd_args?

Indeed also.

splitting on whitespace is probably more natural

It was to be consistent with the other args, but you're right. Space are not valid in an arg, and comma could be.

@legalsylvain
Copy link
Collaborator Author

legalsylvain commented Mar 24, 2020 •
edited
Loading

Hi @sbidoul

  1. First thanks for this great tool ! I began to test it, to have the possibility to use it for GRAP repositories, to use same merge workflow as for OCA. I'm not very comfortable with docker tools and all the technologies used (redis, flower, ...) but I reached to make this tools working in a reasonable time. So thanks ! #AnotherGreatToolByAcsoneTeam.

  2. Regarding this PR, I was thinking to move the folllowing hardcoded values in the environment file also : GITHUB_STATUS_IGNORED, GITHUB_CHECK_SUITES_IGNORED. Ref
    What do you think ?

  3. I was thinking to let also the possibility to rename the bot. Ie : instead of using \ocabot have the possibility to use my new bot with \grapbot. More cosmetic point, but it could be great.

  4. A little bit out of this topic, but I was thinking to add the possibility (in another PR), when launching \ocabot merge to have an extra argument (like minor, patch, etc...) named squach-fixup (not set by default). What do you think ? A lot of time, when I make PR against OCA repository, Once done, I just add fixup commits, to make the review more easy for reviewers. (not all the code to review, only the diff). but finally, when all is ok, I have to return on my PR to squash all the fixup! commits, that is a waste of time.

@sbidoul
Copy link
Member

sbidoul commented Mar 24, 2020

Hi @legalsylvain , Thanks for the kind feedback. Your suggestions make sense. Can you open separate issues to track them? Regarding squashing there is #43 already.

@legalsylvain legalsylvain force-pushed the IMP-add_configurable_settings branch from 715bd6e to f819fb6 Compare March 26, 2020 03:16
@legalsylvain legalsylvain marked this pull request as ready for review March 26, 2020 03:17
@legalsylvain
Copy link
Collaborator Author

Hi @sbidoul. It should be ok now.
it works in production, on GRAP. test.

sbidoul
sbidoul reviewed Mar 26, 2020
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Could you add a file named 103.feature with a changelog entry that mentions the 3 new options? Can you also update environment.sample so people installing following the documentation (which says to copy that file) will discover the new options. [update] you did it already, nice!

@legalsylvain legalsylvain force-pushed the IMP-add_configurable_settings branch from e689a0a to 03f7676 Compare March 27, 2020 18:50
@legalsylvain
Copy link
Collaborator Author

@sbidoul : Done. (my first towncrier fragment!)

sbidoul
sbidoul reviewed Mar 28, 2020
View reviewed changes
newsfragments/103.feature Outdated
@@ -0,0 +1,10 @@
**Features**

Copy link
Member

Choose a reason for hiding this comment

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

The Features section be added by towncrier, so it must not be here. Also, towncrier will transform each newsfragment into a bullet list item, and append a link to the related PR/issue. So this should really be a simple paragraph, and if there is more to say, it should go into the documentation.

sbidoul
sbidoul requested changes Mar 28, 2020
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

LGTM, just needs an update to the news fragment.

@legalsylvain legalsylvain force-pushed the IMP-add_configurable_settings branch from 03f7676 to ea78960 Compare March 28, 2020 09:56
@legalsylvain
Copy link
Collaborator Author

Hi @sbidoul. Thanks for your advices. Fragment updated. Better ?

@sbidoul sbidoul merged commit 26f6eb8 into OCA:master Mar 28, 2020
@sbidoul
Copy link
Member

sbidoul commented Mar 28, 2020

Perfect, thanks!

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

@sbidoul sbidoul sbidoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@legalsylvain @sbidoul