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

#760-Updating Check Changelog and Update Changelog workflows to make it more robust. #816

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

RONAK-AI647
Copy link
Contributor

@RONAK-AI647 RONAK-AI647 commented Nov 7, 2024

The check changelog section and update changelog section is updated to check every field is validated.

Description

Their is an attempt to make the check changelog workflow more robust so it checks that every field of changelog item exist and they are different from their default values.

Issue addressed

1)check for every field of changelog item.
2)they are different from default values
3)they are properly rendered in the PR description.

Addresses #*PR#

Before/after screenshots

A new file is inserted ,see below.
image

Changelog

  • Description: Here is an attempt to make the changelog section more robust .Earlier only the "description" field was mandatory ,but this PR ensures that all the fields of the changelog item exist and anything unnecessary is ignored.
  • Products impact: new API
  • **Addresses:**Make changelog checks more robust #760
  • Components: N/A
  • Breaking: no
  • Impacts a11y: no
  • Guidance: This will check if each field is present and ensures flexibility of PRs.

Steps to test

  1. Open check changelog /workflows/github/kolibri-design-system

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

It was mentioned in "possible tradeoffs" if we could mention all the logic code in a script file to avoid too much of code in gh workflow, an attempt is made to do so.

@rtibbles rtibbles self-assigned this Nov 8, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

A couple of thoughts - I think best to revert the update to the job id - I have two motivations for reusing the Python script, firstly because it's probably more maintainable for us if this is in Python rather than bash, and secondly, so that we don't break things if we tweak the PR template and forget to update the action.

@@ -5,27 +5,10 @@ on:
types: [opened, synchronize, edited, reopened]

jobs:
check-description:
check-pr-fields:
Copy link
Member

Choose a reason for hiding this comment

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

I know it's technically no longer correct - but easier to leave this as check-description so that the required check value stays the same in the repository settings.

And technically we are still checking the PR description here, just in more detail than before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ....i agree then


# Default values for each field(According to PR template)

default_description="Summary of change(s)"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could read these values from the Github PR template instead? We could make the Python script used to generate the changelog reusable, and then use it here to parse the Github PR template, and extract the default values from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok..as you said the python file is more maintainable ,it would make the code more dynamic .But i will need some days to work on it .

Copy link
Member

Choose a reason for hiding this comment

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

No problem @RONAK-AI647, take your time. We appreciate it.

@RONAK-AI647
Copy link
Contributor Author

@rtibbles I made a script file where I stored the codes, should that too be turned to a python file.
If I remove the file and let the codes be in the check changelog....the codes will exceed to 100+ lines.

@MisRob
Copy link
Member

MisRob commented Nov 18, 2024

@RONAK-AI647

@rtibbles I made a script file where I stored the codes, should that too be turned to a python file.
If I remove the file and let the codes be in the check changelog....the codes will exceed to 100+ lines.

@RONAK-AI647 Would you reference for us the file you mean?

@RONAK-AI647
Copy link
Contributor Author

RONAK-AI647 commented Nov 19, 2024

@RONAK-AI647

@rtibbles I made a script file where I stored the codes, should that too be turned to a python file.
If I remove the file and let the codes be in the check changelog....the codes will exceed to 100+ lines.

@RONAK-AI647 Would you reference for us the file you mean?

@MisRob
see the file named "changelog_pr_section.sh" , there the attached screenshot with the PR highlights it.

@MisRob
Copy link
Member

MisRob commented Nov 20, 2024

Hi @RONAK-AI647,

see the file named "changelog_pr_section.sh" , there the attached screenshot with the PR highlights it.

I think you mean this file, right? Then, yes, I believe what @rtibbles is asking here is to turn logic there into Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants