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

Convert issue templates into forms #17855

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Convert issue templates into forms #17855

merged 1 commit into from
Aug 27, 2021

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 26, 2021

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

You can try it here: https://github.com/potiuk/airflow/issues/new/choose - feel free to create new issues there to see how they look like!

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

Screenshots for those who are afraid to create new issues :)

screencapture-github-potiuk-airflow-issues-new-2021-08-26-17_59_57
screencapture-github-potiuk-airflow-issues-new-2021-08-26-17_59_37
screencapture-github-potiuk-airflow-issues-new-2021-08-26-17_59_08

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

Small (but important) addition:

image

@BasPH
Copy link
Contributor

BasPH commented Aug 26, 2021

Love it! Will review for typos, but 💯 for this change!

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

Love it! Will review for typos, but 💯 for this change!

Oh yeah. I really hope it's gone bring the quality of the issues we got up and their number down.

But you never know. The users are creative :D

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

This was also my instinct - I feel like as part of a triage process, we should be able to convert issues to a Meta issue after the fact or to a label.

See my reasoning above @leahecole . I am not super-strong about it, but this is the best balance I found between "easy way to add meta features/tasks" by maintainers and "difficult to enter legitmate issue without providing all the necessary details" by contributors.

Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

Mostly grammar related nits. Really glad you're doing this - thank you so much @potiuk!

.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

Also, should we note in all forms that if you have a code change ready to PR, there is no need to open an issue first.

Good point. I will add it!

@eladkal
Copy link
Contributor

eladkal commented Aug 26, 2021

Not that it will stop some people, but we then will be able to close such issues immediately with "you are not maintainer"

Sounds like a good job for a bot 😎

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

Sounds like a good job for a bot 😎

Oh yeah!

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

I think I addressed all comments. Please take a look @eladkal @leahecole @BasPH @jedcunningham @uranusjr and anyone else interested.

The change depends on #17858 - as during testing I found out some issues with pre-commits validating provider.yaml (and #17858 corrects errors resulting from it)

To make it easier to review, I squashed all the speling/etc fixes into a single commit, and added all new things in the fixup commit that follows. Those are:

  • added separate doc-issue
  • added separate providers issue (with multi-select list of affected providers, yay! - and also added pre-commit to verify if we have all providers there :))
  • added nice preamble to explain that maybe you do not need to create the issue at all
  • added some explanation to versions of airflow (main -> development, 2.1.3 - > latest released version)
  • better sorting of issues in the issue list (bugs go first)

You can try it out here: https://github.com/potiuk/airflow/issues/new/choose

@potiuk
Copy link
Member Author

potiuk commented Aug 26, 2021

And @eladkal -> labels are set properly to doc/core/provider kind of issues. Should help with triaging :)

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Missed one 🤦‍♂️

.github/ISSUE_TEMPLATE/airflow_providers_bug_report.yml Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the form-in-issues branch 2 times, most recently from b995ffd to 7438b66 Compare August 27, 2021 09:17
@potiuk
Copy link
Member Author

potiuk commented Aug 27, 2021

The latest fixup contains last round of grammar/content reviews. I think we are close to a "mergable" status on that one.

I thought hat we might want to be more "welcoming" and "airflow-y" in the issues and here is what I came up with:

Screenshot 2021-08-27 11 15 43

Any more comments? Again - you can play with the latest version here: https://github.com/potiuk/airflow/issues/new/choose

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Great work!

@potiuk
Copy link
Member Author

potiuk commented Aug 27, 2021

I am going to merge it as is, and we can always make corrections later :D

@potiuk
Copy link
Member Author

potiuk commented Aug 27, 2021

[After fixing the static check that is]

@potiuk
Copy link
Member Author

potiuk commented Aug 27, 2021

Rebuilding to check latest constraint works after celery 5 merge.

@potiuk potiuk dismissed leahecole’s stale review August 27, 2021 20:02

I guess all handled ?

@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Aug 27, 2021
@potiuk potiuk merged commit 4d48305 into main Aug 27, 2021
@kaxil kaxil deleted the form-in-issues branch August 27, 2021 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants