Skip to content

Conversation

@amoghrajesh
Copy link
Contributor

Feature newsfragments are change documents that define a new feature being added to Airflow. We should allow some room in there to add some more details about the feature. For instance, one would like to add some working examples.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@amoghrajesh
Copy link
Contributor Author

An example: #46613

I want to add a working example in the feature newsfragment

@jedcunningham
Copy link
Member

I'm a -0.25 on this one. Historically we've only done more than a single line for things that land in the "Significant" block in the release notes. Adding multiline for other sections would mean a different format for our release notes, and until we have a new standard for that, it falls on the poor release manager to sort out as they are building release notes.

I'd say if there is something worth having more than a single line summary about, it should be significant instead. The example you linked would be better as significant, imo.

@Lee-W
Copy link
Member

Lee-W commented Feb 13, 2025

I'm a -0.25 on this one. Historically we've only done more than a single line for things that land in the "Significant" block in the release notes. Adding multiline for other sections would mean a different format for our release notes, and until we have a new standard for that, it falls on the poor release manager to sort out as they are building release notes.

I'd say if there is something worth having more than a single line summary about, it should be significant instead. The example you linked would be better as significant, imo.

Another idea I have (which is probably also how commitizen generates the changelog from longer commits) is adding only the first line to the release note. Rest of them are for ones that cares about the details. so the format would like

.. description of this feature and etc.
.. this line should be empty
.. detail if needed

/

New feature with .....

How to use the feature. the is an example
example 1
......

@amoghrajesh
Copy link
Contributor Author

I'm a -0.25 on this one. Historically we've only done more than a single line for things that land in the "Significant" block in the release notes. Adding multiline for other sections would mean a different format for our release notes, and until we have a new standard for that, it falls on the poor release manager to sort out as they are building release notes.

I'd say if there is something worth having more than a single line summary about, it should be significant instead. The example you linked would be better as significant, imo.

Thanks for the explanation!

I was under the impression that a significant change has to tick one of the boxes under the template:

* Types of change

  * [ ] Dag changes
  * [ ] Config changes
  * [ ] API changes
  * [ ] CLI changes
  * [ ] Behaviour changes
  * [ ] Plugin changes
  * [ ] Dependency changes
  * [ ] Code interface changes

Doesn't seem to be the case with precommit. I think the change is "significant" enough in that case. (One qn, maybe for @Lee-W - does a significant newsfragments only cover "breaking" changes or is it OK to have a significant change that is not a "breaking change"?)

@amoghrajesh
Copy link
Contributor Author

I also like the idea from Wei but seems like its an overkill?

@jedcunningham
Copy link
Member

jedcunningham commented Feb 13, 2025

does a significant newsfragments only cover "breaking" changes or is it OK to have a significant change that is not a "breaking change"?

I intentionally called it "significant" and not "breaking" for a reason :) but that is a subtle nuance, so fair question. (edit: just to be very clear, yes it's fine that it's not breaking! We do it all the time in the helm chart for things like default image versions)

Also, the release manager is the final check - we will sometimes remove or add things to the significant section anyways.

@jedcunningham
Copy link
Member

jedcunningham commented Feb 13, 2025

adding only the first line to the release note. Rest of them are for ones that cares about the details.

Interesting idea. Long term the only place these live is in the release notes themselves - the newsfragment files are deleted once its ends up in a release. So are you suggesting the additional lines are added as rst comments? Not visible in the rendered release notes, but visible if you want to look deeper? Or were you thinking those files lived on post release?

@Lee-W
Copy link
Member

Lee-W commented Feb 13, 2025

I intentionally called it "significant" and not "breaking" for a reason :) but that is a subtle nuance, so fair question. (edit: just to be very clear, yes it's fine that it's not breaking! We do it all the time in the helm chart for things like default image versions)

I thought significant == breaking in this case 😲 But if it's not, we can of course add it to significant.

were you thinking those files lived on post release?

Yep, this is what I thought. I didn't know it would be deleted 🤦‍♂️

So are you suggesting the additional lines are added as rst comments? Not visible in the rendered release notes, but visible if you want to look deeper?

Not actually, but it sounds like a good idea 🤔 I think if we want to keep the detail, it's better to be somewhere in the codebase instead of the github issue

@amoghrajesh
Copy link
Contributor Author

Good ideas @Lee-W and @jedcunningham.

For the time being, I think I am gonna proceed with changing the newsfrag in my PR to a significant and close this one in that sense. But we should definitely follow up and reconsider the idea of adding rst comments. Sounds ok?

@amoghrajesh
Copy link
Contributor Author

Closing this one in favour of the previous comment.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants