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 notes automation #4196

Merged

Conversation

SachinSahu431
Copy link
Contributor

Description

Python script to automate the process of collecting all plugins' components release notes and creating a consolidated release note.

Issues Resolved

Closes #4009

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Add markdown parser 'mistune' in Pipfile
Python script generates 'release-notes availability' table and URL file for verification

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Add custom heading mapping
Fix code format

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
- Handle empty headings
- Support multiple formats
- Fix naming of 'Opensearch.job Scheduler'

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
@peterzhuamazon
Copy link
Member

Reviewed in officehour, suggest @SachinSahu431 to sort the repos based on repo name before adding entries.
Once tests are added we can start reviewing this before adding github actions.

Thanks.

SachinSahu431 and others added 5 commits November 8, 2023 17:55
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Resolve merge conflict
Code cleanup

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <75629410+SachinSahu431@users.noreply.github.com>
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Thanks @SachinSahu431 for the initial draft, ping @gaiksaya for the co-review of this PR.

Two questions:

  1. Could you only update new package in pipfile.lock instead of touching others? I remember able to do that in the past but I dont recall the commands I use. Just to make sure we are not upgrading the existing stable versions of other packages.
  2. You have a lot of .md and .txt in the automation dir, are they needed or just for testing purposes?
  3. Considering adding some sample commands and descriptions to README file within the new dir you created, see build_workflow for example but this can be done later.
  4. Can this be part of the existing release_notes_workflow dir?

Thanks.

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Directory Restructure
Fix Type Checker errors

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
@SachinSahu431
Copy link
Contributor Author

@peterzhuamazon Thank you for the review!

  1. I've undone the modifications in Pipfile.lock and updated it for new package with pipenv lock --keep-outdated.
  2. Affirming that the .md and .txt files are specifically used to validate the results of the ongoing Python script; they will be removed before the merge.
  3. The directory has been reorganized, and the README.md file has been updated.

Thanks again!

Copy link
Member

@gaiksaya gaiksaya 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 the contribution @SachinSahu431 . This will definitely save us a lot of manual effort.
Added some nits and questions. Few suggestions:

  1. Please replace all print statements with logging.info as best practice.
  2. Remove all temp / test files
  3. Fix test cases and add new ones for new methods.

src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/release_notes_workflow/README.md Outdated Show resolved Hide resolved
src/run_releasenotes_check.py Show resolved Hide resolved
src/run_releasenotes_check.py Outdated Show resolved Hide resolved
src/run_releasenotes_check.py Outdated Show resolved Hide resolved
@gaiksaya
Copy link
Member

Hi @SachinSahu431

Please feel free to convert this PR ready for review when you are ready. Looks like tests needs to be fixed and also some comments need to be addressed.
Thanks!

Add 'results' to gitignore
Fix nits
Add manifest file for tests

Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Attention: 132 lines in your changes are missing coverage. Please review.

Comparison is base (a97cf7d) 93.17% compared to head (150c5d4) 91.24%.
Report is 18 commits behind head on main.

Files Patch % Lines
src/run_releasenotes_check.py 5.60% 118 Missing ⚠️
src/release_notes_workflow/release_notes.py 52.94% 8 Missing ⚠️
.../release_notes_workflow/release_notes_component.py 70.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
- Coverage   93.17%   91.24%   -1.94%     
==========================================
  Files         188      188              
  Lines        5920     6063     +143     
==========================================
+ Hits         5516     5532      +16     
- Misses        404      531     +127     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SachinSahu431
Copy link
Contributor Author

Hi @gaiksaya!

I have added tests for the new compile option. Explicit tests for the check option are not required as it is a subset of the compile option. Also, since the consolidation script ignores OpenSearch component, I've created a new manifest file with component named Opensearch-test in manifests/templates/opensearch/test for testing purposes.

@SachinSahu431 SachinSahu431 marked this pull request as ready for review November 19, 2023 18:09
Signed-off-by: Sachin Sahu <sachinksahu.431@gmail.com>
@bbarani
Copy link
Member

bbarani commented Dec 1, 2023

@gaiksaya @peterzhuamazon Can we move ahead with this PR?

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Looks like code coverage as commented on the lines missing the test cases. Is it possible for you to add those?

If not, you can take that up as a follow up PR.

@@ -37,3 +37,4 @@ out.txt
vars/

test-report.yml
src/release_notes_workflow/results/*
Copy link
Member

Choose a reason for hiding this comment

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

Is this still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaiksaya Yes, this is to ignore the folder containing temporary release notes created by script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can work on missing test cases in a new PR.

@gaiksaya gaiksaya merged commit 5dbcbaf into opensearch-project:main Dec 6, 2023
11 of 12 checks passed
@gaiksaya
Copy link
Member

Hey @SachinSahu431

Looks like a bug is introduced by this PR #4549
Please feel free to pick up the issue if you are interested.

Thanks!

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.

Automate consolidated release notes creation
4 participants