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

Release tool improvements #9008

Closed
2 of 7 tasks
nawazkh opened this issue Jul 18, 2023 · 15 comments
Closed
2 of 7 tasks

Release tool improvements #9008

nawazkh opened this issue Jul 18, 2023 · 15 comments
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@nawazkh
Copy link
Member

nawazkh commented Jul 18, 2023

This issue aims to track the improvements needed/done at release tool
Assuming this to be a long-term effort, this issue will be used as an epic and subsequent tasks will be created to track the changes.

Release Tool Improvements

  1. kind/bug triage/accepted
    Dhairya-Arora01
  2. kind/bug triage/accepted
    Dhairya-Arora01
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 18, 2023
@killianmuldoon
Copy link
Contributor

/triage accepted

I don't think there's any reason to open up seperate issues for each of these as they're quite small and multiple might be done in a single PR.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 18, 2023
@nawazkh
Copy link
Member Author

nawazkh commented Jul 18, 2023

/triage accepted

I don't think there's any reason to open up separate issues for each of these as they're quite small and multiple might be done in a single PR.

Oops, sorry! I created a few already; did not see your reply here.
Feel free to manage the other issues, if you would like to close them out or keep it as is. :)

@g-gaston
Copy link
Contributor

@nawazkh can we add another one to the list?

When there are multiple areas, areas should be replaces with a well formatted version of their current rendering
clustercachetracker|provider/control-plane-kubeadm|provider/bootstrap-kubeadm
To
ClusterCacheTracker/KCP/CAPBK.

@killianmuldoon
Copy link
Contributor

When there are multiple areas, areas should be replaces with a well formatted version of their current rendering
clustercachetracker|provider/control-plane-kubeadm|provider/bootstrap-kubeadm

I think we have to be careful with this as these probably do need to be manually reviewed to be sure there aren't issues with the multiple areas added. Maybe we could add another marker to signal that multiple areas are okay for a given PR so we don't have to manually fix it up each time we generate the notes.

@g-gaston
Copy link
Contributor

g-gaston commented Jul 19, 2023

I think we have to be careful with this as these probably do need to be manually reviewed to be sure there aren't issues with the multiple areas added.

Yeah, for know I'm suggesting just making sure we do the provider/control-plane-kubeadm -> KCP replacement
Same overall format with MULTIPLE_AREAS[*]. So it will still need review but at least it's just about deciding what to keep and what don't, but the areas themselves are user friendly.

Maybe we could add another marker to signal that multiple areas are okay for a given PR so we don't have to manually fix it up each time we generate the notes.

Good point. I would suggest tackling this as a separate task. It seems we might need to introduce a new mechanism, so it's not as cheap as just doing the replacement and might need a bit more thought. Maybe we can revisit based on how frequent these are? Maybe they are rare and it's not worth the added complexity.

@killianmuldoon
Copy link
Contributor

Yeah, for know I'm suggesting just making sure we do the provider/control-plane-kubeadm -> KCP replacement
Same overall format with MULTIPLE_AREAS[*]

That sounds perfect! Let me know if the task I added above captured what you want.

Maybe we can revisit based on how frequent these are? Maybe they are rare and it's not worth the added complexity.

Completely agreed - I'll add a generic task for this on this umbrella issue, but I think you're right that it's not very high priority.

@sbueringer
Copy link
Member

sbueringer commented Jul 20, 2023

Just posting here because it's related. Going forward let's please consider that the release note tool can also be used by non-core CAPI. I.e. we should be careful about hard-coding the cluster-api repo name or adding features (without flags) that don't make sense for other consumers (xref: #9018 (comment))

P.S. the area mapping is totally fine

@g-gaston
Copy link
Contributor

Just posting here because it's related. Going forward let's please consider that the release note tool can also be used by non-core CAPI. I.e. we should be careful about hard-coding the cluster-api repo name or adding features (without flags) that don't make sense for other consumers (xref: #9018 (comment))

P.S. the area mapping is totally fine

Should we add a README to the folder stating this? Or any other way to document this goal that you see fit.

To be honest, I didn't know this was a goal of the tool. So I fear if we don't document it, someone else might come in the future without this background and make breaking changes again.

@sbueringer
Copy link
Member

sbueringer commented Jul 20, 2023

README sounds like a good idea.

To be honest I don't know if it was an explicit goal of whoever forked the kubebuilder release note tool initially :).

But I think it's reasonable, we just have to be aware of it and then we can write new features in a way that they either work generically or can be toggled

P.S. I'm mostly thinking about CAPI providers leveraging this tool as they are closely related

@killianmuldoon
Copy link
Contributor

Let's also add this as a comment on main.go in the release notes tool as an additional hint as maintenance of this tool is likely to swap hands frequently with release team handovers.

I also wasn't aware that we were considering non-CAPI users of the tool, but it makes complete sense for providers at least.

@killianmuldoon
Copy link
Contributor

After #9018 merges:

From comment:

we should add a task to #9008 to add a make tool target that builds the release-notes tool if not present and runs it with the expected args. We should also update this guidance in the release docs.

@sbueringer
Copy link
Member

Just to share data. Currently used by 4 other providers (soon 5 with CAPV): https://cs.k8s.io/?q=sigs.k8s.io%2Fcluster-api%2Fhack%2Ftools%2Frelease&i=nope&files=&excludeFiles=&repos=

@killianmuldoon
Copy link
Contributor

@kubernetes-sigs/cluster-api-release-team to make sure I captured all the issues we spoke about on today's call.

@g-gaston
Copy link
Contributor

/close
this one in favor of #9104

@k8s-ci-robot
Copy link
Contributor

@g-gaston: Closing this issue.

In response to this:

/close
this one in favor of #9104

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants