Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[OSCI][FEAT] Changelog Project - PoC Changelog and release notes automation tool - OpenSearch Dashboards #5519
[OSCI][FEAT] Changelog Project - PoC Changelog and release notes automation tool - OpenSearch Dashboards #5519
Changes from 167 commits
915ac64
a19b527
4efcc6d
245b559
6b74097
034b742
d82cfca
bdaba2f
ec41ec0
e384081
621aa2f
f65914a
f951e7b
e7f10ce
2c4c1de
5460b25
d43c874
78aabf6
8a1b35f
9cb4afb
e7fe1bb
80eb6f2
4bd6da8
7cb63de
34cfb78
45072e8
698398d
9cc2a72
c0b93e0
f715f53
5c37391
ad76b43
3ece4eb
04bdb2b
d5fe142
894430e
70dbb55
5b9b939
1211af2
9b8a07f
ef3f61e
981ef3e
c1945a9
97c2fd6
c777b66
f65ba39
4775d6f
81f1c59
89337de
7620ffa
7a567c3
31232db
86f10bd
c3843c6
72c3d89
204c363
3b0b2f2
a875300
941dd6c
efc3948
9433869
b80f45f
6795bc0
7ba8a70
da6ad8e
6e9a401
efdbf39
941bf62
36e3fd5
f918897
afc82b3
2e82bd2
3af0f22
a1a3f20
73ce52f
7d9145a
e907bc8
79813cb
db0024e
2090f1c
b200102
c6bcd39
3bd3ad6
48e6b56
c24773b
616290f
2d2a425
862b72b
dc9d3c8
4f63874
aef2861
f359dfb
c258342
d117bd5
996b6ec
ced0e10
9387e34
4ec6548
0c4a141
d736d6b
69bb1c6
bc988e7
925ee36
8d456ff
59b7095
53e5445
44958fc
53bf365
e4798bb
a7ed3c0
2b61175
97eb4c9
bc888ec
49faeef
1b43e42
8bb008b
0b56774
4f4a642
6285dca
67200d2
9f508f5
58dbc69
cbf5689
affdf8d
62739b7
b81044c
0095aea
bc26ea1
83acd9a
51dc42f
e8f4fbb
8899405
8abb3a8
13609ce
7e2e496
f7fbc1c
57bc3f5
98a5861
80ffcfe
822a672
17978fc
a9ca159
21b227f
753bd96
6df3825
fdbf8dd
d8650c2
9adce3f
e4ec9b9
81f0638
ddd615d
920e5f8
9b37d02
8853256
b2db67b
c3a2b5d
d198383
3eaa438
e657702
975f86b
0bb4a9c
ef41272
f5d2d1d
2005a2f
f0853fd
140641b
267f7f4
d6c0986
7fe157b
2281d93
afdf7c9
7a52535
15698e0
2d2e416
ddc6050
34ed89f
23bf91c
c44f17d
b53e5e3
5410f39
f7037ba
cf3c334
f5e2867
4e5ad9e
462075b
63327bc
5b59894
d866570
87eaf8d
4dfd118
90dbe73
fd6654f
85f0e6c
c66a475
67bd0a9
ad95462
6826f5d
04661af
1237f33
5157cd8
3f4d7e3
b3df299
f7f1e5a
bebf437
84441e3
a416e4e
486d0a7
27ba900
ba6131a
41afaff
cb50b64
f6c23e7
cc29538
b1e14d6
e4ca375
d36d802
fcd61df
c5df36e
cb06b4d
9b1c1a6
137d94d
a677ecc
33c1f86
3219956
4e94689
63354a3
9145f3e
ae8cd89
234bcb8
2c4783c
d6149af
8599c93
c0d9fe6
44ddd43
957a1b4
e57bb57
65f093d
0fe81c4
9fc3d37
49cd02d
469ef07
3b0506a
9f9a25d
fc9ed97
9688c2f
c843d24
c154db8
6940574
b0bb2ef
e4c81c2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be triggered to generate the change set on every PR open/edit; however, you can't use a GHA on in OpenSearch-Dashboards to update the remote fork - think of all the possible security issues. However, I think there are a couple ways you could handle this:
Triggered a GHA on PUSH that is only run on fork repositories - where you attempt to find a linked PR and then pull the description and commit changes on that fork - where you've got permissions needed to do all of that. This will require forks to enable GHA, and might be a little confusing for first time contributors.
Trigger a GHA on MERGE of a PR in Dashboards, then you can find the PR description with the changelog comment and push an update to the change set. I think you might need to update the backport job to pickup the relate change commit entry.
Happy to help troubleshoot issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @peternied! The first option is more suitable for us, given that we want to avoid backport problems. Regarding your suggestion, I have some questions for you.
Let's say I push a commit from my fork but still need to create a PR. How do you think you could handle this case? To run the workflow endlessly until it finds a PR? I think that is relatively inefficient. We require a GHA event that can trigger an event in the fork repo when a PR is opened or updated in the base repo. But I still can't find a suitable one. I thought we could use the GHA event
workflow_dispatch
, but annoyingly, this one would require a PAT token from the owner of the fork repo (the contributor) to be added to the base repo.Also, do you know if there is a change to grant maintainer permissions to a GHA. This is because, when a contributor submits code by a PR from a fork repo to a base repo, a default option is already activated where a maintainer is able to make commits back to the branch owned by this same contributor. This is something that is available but only for manual operations from a maintainer of the base repo. But what if we could make a specific GHA as a "maintainer" too. That would solve our problem! But I think this option is not available.
Any suggestions?
Thanks for your help again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the fork, on push you'll need to trigger an action that attempts to correlate the current commit with pull requests on the destination repo - if no PR is found I'd terminate the workflow, not sure what other options you'd have.
Looks like there is an API that would let you do this, try https://api.github.com/search/issues?q=repo:opensearch-project/opensearch-dashboards+type:pr+SHA:13609ce6c051f557a7fc7b52a6ca8037d81f3e9a
Trigger this on every push to the fork should be enough?
I'm not sure if a PAT would be needed, since the 'write' operations would be limited to the fork they'd be able to run with write permissions. Note, you might run into an issue where forks GHA's are getting throttled.
I want my fork(s) to be fully under my control - I would be highly suspicions of any workflow that required this permissions. I would recommend finding another path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @peternied . Thank you for your feedback and suggestions! I think the dilemma we are facing is (1) how to maintain security best practices for both OpenSearch repos and forked repos while also (2) simplifying the process—for both maintainers and contributors—of generating changelog entries for PRs.
Regarding your suggestion about triggering a GHA on push that is only run on forked repositories, I like the idea. I'm just trying to think about possible issues that could arise if we went this route. You and @BigSamu have already discussed a couple of them (what to do if there is no PR for the workflow to correlate the commit with and potential issues with GHA throttling).
Another scenario I'm envisioning is if a contributor fails to create separate branches in their forked repo for issues they are working on. This could lead to ambiguity when the workflow tries to identify the correct PR for the changeset file. Any thoughts about how we might handle an edge case like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied, thanks for your replies. Below are my comments.
The issue is the following: what if a contributor commits an update of the code in the branch of his fork and then opens a PR with no new commits submitted later (in this scenario, the maintainer considers the 1st version of the new code to be correct and no further actions are required)? Then the changeset files for this PR will never be created automatically in the branch of the fork.
Thanks for the API information. This is useful to have into consideration.
Imaging the case I gave you before. To just run the workflow upon a push event is not enough. Also, I think that it is not efficient in terms of resources. We just want to trigger an action in the forked repo, when a PR is opened or edited into the base repo.
In this case, I mean that a PAT will be required when sending an API call with specific information from the base repo (upon a
pull_request
event) to the forked repo. The latter one will only be able to run a workflow for aworkflow_dispatch
event related to the API call if there is a valid PAT token coming on it.Indeed. But for example you as owner of a fork can grant permissions to the mantainer to make commits on a branch you use to open a PR. Wondering just for this specific case to have a workflow with those permissions.
As an alternative approach, what do you think about using a Github App. I understand that this could overcome our problem; however, it implicates more work. Currently, we have the following reusable workflow that performs all our logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BigSamu @JohnathonBowers I would caution about striving for perfection when the current system is far from ideal. Maybe as is this change is 50% better than the status quo so ship it? Then when/if there is more time it can be further enhanced.
Speaking greedily - I'd want this tool available for OpenSearch and my other repositories even if it isn't fully automated. It would be a huge improvement to avoid merge conflicts when backporting change alone - even if me/other maintainers are still editing changeset descriptions on a file system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied, make sense what you say! We will keep you updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, it also seems their might be a lack of standardization repo by repo within the project so I also don't see the need to block this if this make the CHANGELOG issue improve even a little bit for the sake of handling every case. Especially if it blocks more contributions and creates conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peternied, @kavilla,
At last, we solved our problem, moving the logic to a GitHub App. The PoC is fully functional, as we have envisioned since the beginning. The only need for a contributor would be installing the newly created GitHub App, OpenSearch-bot. We will do a demo today with @joshuarrrr and @ashwin-pc to present the functionality.
We will keep you updated for the roll-out of this app.
Regards,
Samuel
PS: You are more than welcome to review the code for our Github App and collaborate if you want. Here the link to the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the action repo under opensearch-project? @ashwin-pc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can in future, but lets first start using it and see how effective it is. Once we have more repo's adopting this we can start that process. I dont want to hold this PR for just that reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Issue:
This GitHub action could be updated to inject unexpected content into this repo's workflows, see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions
Fix is to publish v1 of OpenSearch_Change_Set_Create_Action and switch to that tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release note generator doesn't really depend on anything. As such, I would have preferred for it to be in vanilla js so we can execute it without the need to bootstrap the project and deal with possible issues of bootstrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS, the wordnotes
is plural.Nah. this one generates one at a time, so it must be singular
note
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before. Are you suggesting I change everything to javascript instead of typescript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CMDWillYang can you check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe at the start of this project a maintainer suggested using typescript, I will need to do some digging to look up their reasoning. But @AMoo-Miki makes a great point considering the problems of running ts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a fair point. Would be difficult to move everything to vanilla JS? At least the code you build with Riley was not to much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashwin-pc thoughts on this? I'm happy to migrate to js if we decide that the ease of running js outweighs the benefits of ts.