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

Cleanup README #211

Merged
merged 1 commit into from Sep 29, 2020
Merged

Cleanup README #211

merged 1 commit into from Sep 29, 2020

Conversation

mdellweg
Copy link
Member

  • Removed spurious paragraphs
  • Removed outdated sections
  • Updated completeness checklist

fixes #6474
https://pulp.plan.io/issues/6474

Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

I think it would be good to have a section explaining the --all arg, and what we need to do to make it work, like here: https://github.com/pulp/plugin_template/blob/master/.travis/bootstrap_catdog.sh#L3-L9

Comment on lines -435 to -469
The pipeline can be modified, see the help text for available options.

```
$ ./plugin-template --help
usage: plugin-template [-h] [--generate-config] [--bootstrap] [--test]
[--travis] [--docs] [--all] [--verbose]
plugin_name plugin_app_label

Generate a .travis.yml and .travis directoryfor a specified plugin

positional arguments:
plugin_name Create or update this plugin.
plugin_app_label the Django app label for the plugin - usually the part after pulp_ or
pulp-


optional arguments:
-h, --help show this help message and exit
--bootstrap Create a new plugin and template boilerplate code.

--test Generate or update functional and unit tests.

--travis Generate or update travis configuration files.

--docs Generate or update plugin documentation.

--all Create a new plugin and template all non-excluded files.

--verbose Include more output.

```
Copy link
Member

Choose a reason for hiding this comment

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

why removing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to remember, why:
It is outdated, and the information here is doubled anyway. It looked to me like a stop gap until better content was written.

@mdellweg
Copy link
Member Author

What is "--all" doing? Is it different from "--bootstrap"? I guess, it at least includes "--bootstrap" which does horrible things on an existing plugin.

@fao89
Copy link
Member

fao89 commented Apr 28, 2020

What is "--all" doing? Is it different from "--bootstrap"? I guess, it at least includes "--bootstrap" which does horrible things on an existing plugin.

--all executes all args: bootstrap, travis, test,...
as the issue is about out of the box, --all is what we have that is more close to it.
as you mentioned about --bootstrap, --all would be terrible for an existing plugin.

Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

Thank you @mdellweg

README.md Outdated
Comment on lines 194 to 199
At this point, you have a one off chance to use `--all` instead of `--bootstrap` to include all the
other sections described below.

**Note** : Regenerating the *bootstrap* section at a later time will reset all files to their
original state, which is almost always not intended.
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link

Choose a reason for hiding this comment

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

I think we need to expand upon this a little bit. Can we try and elaborate on what --all means here, rather than saying it includes other sections described below? If I use --bootstrap, I get a minimum viable plugin that I can then customize(I'm guessing), if I run --all, what specifically do I get?
The fact that this is an option means that someone might want to use it. Who is this person and what would be their aim? Could we add an example of when it would be an advantage to use --all rather than --bootstrap at this point, since it is their one chance to do this?

Copy link
Member

Choose a reason for hiding this comment

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

  --generate-config     Create or update a plugin template config for a plugin and exit.
                        
  --bootstrap           Create a new plugin and template boilerplate code.
                        
  --test                Generate or update functional and unit tests.
                        
  --travis              Generate or update travis configuration files.
                        
  --docs                Generate or update plugin documentation.
                        

you can start a plugin interactively:
0 - ./plugin-template --generate-config --plugin-app-label catdog pulp_catdog
1 - ./plugin-template --generate-config --plugin-app-label catdog pulp_catdog
with the previous steps, you can have a minimum viable plugin, and choose whether you add tests, CI or docs.
Note: step 0 is mandatory, as plugin_template depends on the config
So, if a week later I want to add tests I can:
2 - ./plugin-template --test pulp_catdog

If the plugin writer wants a full plugin since the beginning, --all would be useful as it executes (bootstrap, test, travis, docs). In our CI we want to build a full plugin for testing, so we use --all:
https://github.com/pulp/plugin_template/blob/master/.travis/bootstrap_catdog.sh

Copy link

Choose a reason for hiding this comment

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

How about something like:

At this point, you have a one-off opportunity to use the --all option, which generates everything included in the --bootstrap option, as well as documentation, functional and unit test, and travis configuration file templates that you require to support a plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. I will take that and leave the decision whether --all should work differently open.

@@ -172,6 +170,8 @@ The following settings are stored in `template_config.yml`.
This is used during commit validation to make sure the commit is attached to
an issue in the correct project.

pulp_settings A dictionary of settings that the plugin tests require to be set.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know it was not documented yet, thanks!

* Removed spurious paragraphs
* Removed outdated sections
* Updated completeness checklist

fixes #6474
https://pulp.plan.io/issues/6474
@quba42
Copy link
Contributor

quba42 commented Sep 28, 2020

Just saw this PR. Is this ready to go? (It might be better for me to work on #272 on top of this change.)

@pulpbot
Copy link
Member

pulpbot commented Sep 28, 2020

Attached issue: https://pulp.plan.io/issues/6474

@mdellweg
Copy link
Member Author

I'd be happy to merge it right away.
Ping for awareness: @fao89

@fao89
Copy link
Member

fao89 commented Sep 29, 2020

I'd be happy to merge it right away.
Ping for awareness: @fao89

I've been waiting for this, please merge it!

@mdellweg mdellweg merged commit a19bf51 into pulp:master Sep 29, 2020
@mdellweg mdellweg deleted the cleanup_readme branch September 29, 2020 13:07
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.

None yet

5 participants