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 issue templates for new requirements #125

Merged
merged 5 commits into from
May 19, 2023
Merged

Conversation

nuclearsandwich
Copy link
Collaborator

Related to #106.

Lots of folks have been checking the boxes here when they submit the
template, however my intention was that they would populate the
incomplete checklist and the ros2-gbp admins would check those off when
they import the repo. We'll see if this comment improves the situation
or not. If it doesn't I may remove the checklist from the template
entirely.
As discusesed, we want repositories to undergo naming and license review
as source packages before we create release repositories for them.

Right now this process is quite manual but building tools which
streamline the workflow is planned.
@nuclearsandwich
Copy link
Collaborator Author

@ijnek what do you think of these changes? I think we need to enhance the contributing docs in ros/rosdistro with some explicit cases for this as well but I started these updates first.

Copy link
Member

@ijnek ijnek left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, these changes look good!

@nuclearsandwich nuclearsandwich merged commit a17340c into latest May 19, 2023
@nuclearsandwich nuclearsandwich deleted the update-templates branch May 19, 2023 15:53
Copy link

@130s 130s left a comment

Choose a reason for hiding this comment

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

Left some feedback for the template based on the conversation in #275

* [FIRST_REPOSITORY](SOURCE URL)
* [ros/rosdistro source entry](LINK TO SOURCE FIELD IN ROS/ROSDISTRO)
* [ ] There is an existing release repository which should be imported: <RELEASE REPOSITORY URL>
* [SECOND REPOSITORY](SOURCE URL)
Copy link

Choose a reason for hiding this comment

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

How about making this 2nd repo entry option, or simply delete? This made me go through

"why 2nd repo is asked here...Hm, maybe there are people who want to submit a single request for multiple repos, but I'm not sure how common that would be?",

and finally I just made a single entry 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I did assume that many "new" requests for ros2-gbp repositories would be in batches of two or more, but that probably only made sense when the ros2-gbp org project was new itself and adding repositories to an existing team probably is a one-at-a-time deal except in cases where a new suite of packages in separate repositories is being released for the first time.

My general rule of thumb for templates is that you should show how to sequence things that can be duplicated and I would much rather one issue with two repositories than one issue per repository.


If there are any existing release repositories which should have their contents imported into the ros2-gbp organization, list and link to them here.
Leave the checkbox _unchecked_. The ros2-gbp administrator will check the box when they have completed the repository import.
-->
* [FIRST_REPOSITORY](SOURCE URL)
Copy link

Choose a reason for hiding this comment

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

Given a PR to rosdistro that lists up required info e.g. source repo URL is a must, shouldn't a link to such PR suffice? If so, we can get rid of [FIRST_REPOSITORY](SOURCE URL) line, for simplification?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a great point! When the template was first written the rosdistro PR was not yet a requirement. It's slightly "more work" for the maintainer to have to follow the the source url through the rosdistro PR but I would much rather do that work in order to simplify the template! Thanks for the feedback!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks again for the feedback, I've incorporated it into a new draft PR here #296 but I'm also considering a larger restructuring of the documentation for release repository creation.

* [FIRST_REPOSITORY](SOURCE URL)
* [ros/rosdistro source entry](LINK TO SOURCE FIELD IN ROS/ROSDISTRO)
Copy link

Choose a reason for hiding this comment

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

I didn't understand what "LINK TO SOURCE FIELD" points to.

Should a URL to a PR that is already opened in rosdistro be sufficient? With that, it'll be ros2-gbp maintainer's job to find the info that this line is currently asking for unfortunately, but as a total that might reduce confusion and make the process smoother.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this step could step was confusing both from the instructions on docs.ros.org here:

I think it should suffice to have a link to an open PR otherwise the process could just take too long, waiting for maintainers of both ros/rosdistro and ros2-gbp/ros2-gbp-github-org

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 think it should suffice to have a link to an open PR otherwise the process could just take too long, waiting for maintainers of both ros/rosdistro and ros2-gbp/ros2-gbp-github-org

Thanks for the feedback. Just opening the ros/rosdistro PR is not sufficient to indicate that a newly added repository has actually passed any kind of review, so this change to the process would not be sufficient and we would end up with the same cases which caused us to introduce the source repository requirement in the first place: packages being submitted which do not meet rosdistro naming requirements, needing a name change, and having to do that name change after having already created a ros2-gbp release repository.

The current process is clunky and far from optimal. I have been working on a new bloom subcommand which automates submitted PRs for source repository indexing in the same fashion that it can currently automatically create release PRs and one potential feature of this project in the future is the automatic creation and deployment of ros2-gbp release repositories as soon as the source repository is added to ros/rosdistro. I appreciate the feedback immensely because it helps me better understand which areas of the process are the most in-need of optimizations.

nuclearsandwich added a commit that referenced this pull request Jul 1, 2023
This is a start at addressing some of the feedback provided in #125

My objective in using a big issue template with lots of explanatory
comments was to make the documentation as local to its use as possible
without cluttering the resulting issue (after all, the submitter can't both read the formatted text and fill out the template at the same time).
But as I iterate, I'm starting to think that [Issue
Forms](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms)
and a link to guiding documentation would be easier to follow and result
in stil cleaner issues than this format. So I may abandon this effort in
pursuit of a wider refactor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants