-
-
Notifications
You must be signed in to change notification settings - Fork 88
[feature] Added guided release tool #497
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
Conversation
cc90865 to
3a7aa9a
Compare
nemesifier
left a comment
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.
Looks like it's slowly moving in the right direction.
See my comments below.
tests/test_generate_changelog.py
Outdated
| Changelog | ||
| ========= | ||
| [unreleased] |
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.
There's the version and date?
nemesifier
left a comment
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.
Great to see progress on this one! 👍
See my comments below and in the rest of the PR.
- How can the script be used? We need to summarize the key points of this logic in the docs, please add a new page in the dev section. See other similar pages for reference, eg: QA checks, Re-usable GitHub Actions and Workflows.
- Why did you place the files into
/releaserinstead ofopenwisp_utils/releaser?
It's not necessarily wrong but I want to know if there was a specific purpose for this? It could make sense to ship this logic only during development and not for production code. - Where is the logic which makes the script available to users installing this module? See
openwisp-qa-check,openwisp-qa-format,openwisp-pre-push-hook,checkmigrationsandcheck_commit_messagefor reference.
| [deps] Update django requirement from ~=4.0 to ~=4.1 | ||
| [deps] Update requests requirement from <2.25 to <2.30 | ||
| [test] Add tests for the new feature | ||
| [deps] Update django requirement from ~=3.2 to ~=4.0 |
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.
Can you include in these samples full git log output which inlcudes extended commit messages (spanning multiple lines)? We need to make sure the change log incorporates the full git log.
docs/developer/index.rst
Outdated
| ./test-utilities.rst | ||
| ./other-utilities.rst | ||
| ./reusable-github-utils.rst | ||
| ./releaser-script.rst |
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.
| ./releaser-script.rst | |
| ./releaser-tool.rst |
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.
Did you forget to update this?
docs/developer/releaser-script.rst
Outdated
| @@ -0,0 +1,91 @@ | |||
| The Releaser Script | |||
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 Releaser Script | |
| The Releaser Tool |
docs/developer/releaser-script.rst
Outdated
|
|
||
| .. include:: ../partials/developer-docs.rst | ||
|
|
||
| This script automates the project release workflow, from changelog |
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 script automates the project release workflow, from changelog | |
| This tool automates the project release workflow, from change log |
| "git-cliff~=2.10.0", | ||
| "questionary~=2.1.0", | ||
| "pypandoc~=1.15", | ||
| "pypandoc-binary~=1.15", |
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.
If the user hasn't installed these by running pip install openwisp-utils[releaser], the script will fail right? Are you checking for these in the pre-requisites check?
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.
Yes, it fails.
| result = get_gpt_summary(SAMPLE_CONTENT, "rst", token=None) | ||
| assert result == SAMPLE_CONTENT | ||
| mock_print.assert_called_with( | ||
| "⚠️ CHATGPT_TOKEN environment variable is not set. Skipping AI summary.", |
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.
nemesifier
left a comment
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.
Summary of manual testing.
Resilience
We need the tool to be resilient: any step, especially the network requests, can fail. If a failure happens after 50% of progress, we can't repeat the process, rolling back or recovering is time consuming. The tool must be ready to catch failures and recommend manual actions.
I recommend adding something like retriable_request, a function that can execute another function and provide a fallback action to recommend in case of errors.
This means that it would be great to make sure every step in the process has its own function.
Error ouput
When there's an unexpected error/exception, it would be good to print out as much error information as possible to ease debugging.
openwisp_utils/releaser/release.py
Outdated
| else: | ||
| print("Skipping changelog port. Please remember to do it manually.") | ||
|
|
||
| except KeyboardInterrupt: |
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.
Can we move these try/except to the if __name__ == "__main__": block?
openwisp_utils/releaser/release.py
Outdated
| return config, gh | ||
|
|
||
|
|
||
| def get_gpt_summary(content, file_format, token): |
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 could use a more generic naming here:
| def get_gpt_summary(content, file_format, token): | |
| def get_ai_summary(content, file_format, token): |
openwisp_utils/releaser/release.py
Outdated
| from openwisp_utils.releaser.github import GitHub | ||
| from openwisp_utils.releaser.version import bump_version, get_current_version | ||
|
|
||
| MAIN_BRANCHES = ["main", "master"] |
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.
Master is more common:
| MAIN_BRANCHES = ["main", "master"] | |
| MAIN_BRANCHES = ["master", "main"] |
| ) | ||
|
|
||
|
|
||
| def main(): |
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.
Can we move this to the top to ease readability?
openwisp_utils/releaser/release.py
Outdated
| return content | ||
|
|
||
|
|
||
| def determine_new_version(current_version_str, current_type, is_bugfix): |
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.
Why not move it to version.py?
openwisp_utils/releaser/release.py
Outdated
| run_git_cliff, | ||
| ) | ||
| from openwisp_utils.releaser.github import GitHub | ||
| from openwisp_utils.releaser.version import bump_version, get_current_version |
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 like how you separated logic in different files here.
I recommend checking if it moving more of the functions from this file to other files could improve readability.
| @@ -0,0 +1,186 @@ | |||
| import importlib.resources as pkg_resources | |||
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.
can we rename this file to just changelog.py?
releaser.toml
Outdated
| @@ -0,0 +1,3 @@ | |||
| repo = "Dhanus3133/openwisp-utils" | |||
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.
Is this file still needed?
docs/developer/index.rst
Outdated
| ./test-utilities.rst | ||
| ./other-utilities.rst | ||
| ./reusable-github-utils.rst | ||
| ./releaser-script.rst |
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.
Did you forget to update this?
| @@ -0,0 +1,16 @@ | |||
| [feature] Generate CHANGES.rst automatically #496 | |||
| [change] Switched to prettier for CSS/JS linting #367 | |||
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.
What if a commit message spans multiple lines? Are we omitting the long description? Let's make sure long descriptions are included please.
nemesifier
left a comment
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 tried this now for this: openwisp/ansible-wireguard-openwisp#71
The releaser.toml file got included in the commit:
Is this normal? I don't think this should happen.
I'll remove it manually for now.
|
I don’t think PS: I think we could also add logic to notify the user about other changes that are available locally ater the checks. |
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 don’t think
releaser.tomlis created automatically. From what I remember in the call, we were testing with the previous implementation (releaser.tomlway), so I believe it was created at that time. Are you sure if there were no unstaged changes already present before running the release now?PS: I think we could also add logic to notify the user about other changes that are available locally ater the checks.
I had to make some changes after the changes were created before proceeding: I had to reformat files due to the fact that we updated openwisp-utils to use prettier also for JSON and YAML files. So I pushed this change first, then I went ahead with the process after verifying the change log appeared.
Question: is it possible to avoid creating the file at all or is it really needed? In the worst case scenario we can add a .gitignore file in each repo although this will be time consuming.
Ansible WLP testing error due to rst syntax issue
I tried this here: https://github.com/openwisp/ansible-openwisp-wifi-login-pages.
This is what happened:
Warning: Could not format content with `docstrfmt`.
WARNING: File "/tmp/tmpbw5uy424.rst", line 27:
Bullet list ends without a blank line; unexpected unindent.
Failed to format '/tmp/tmpbw5uy424.rst'
1 file was checked.
Done, but 1 error occurred ❌💥❌
This is probably caused by a restructured text syntax error somewhere which is crashing docstrfmt. However, it's not clear whether I can recover from this error manually.
When this happens, it would be good to let the user know about the problem and return the error but we should also return the unformatted change log output, so the user can try figuring out where the problem is or in the worse case scenario overcome the issue by accepting the unformatted change log and then solving it manually before the final acceptance.
I am sure it was not created automatically. I tested out locally now. You can check this Dhanus3133/ansible-wireguard-openwisp#1 out. |
Weird, I tried this on my fork now Dhanus3133/ansible-openwisp-wifi-login-pages#1 didn't seem to encounter this issue.
Okay, understood. |
Maybe it was a leftover from one of our previous tests. I hope you can make the initial change log generation robust to failures in case |
… porting changelog
ac09af9 to
00a9816
Compare
nemesifier
left a comment
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.
My last attempt worked smoothly, I'll be doing a few more releases before merging but it looks really good now, thanks!
| changelog_date_str = datetime.now().strftime("%Y-%m-%d") | ||
| tag_date_str = datetime.now().strftime("%d-%m-%Y") |
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.
@nemesifier did you notice that we have two different date formats for tag and changelog?
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.
No, but it's not a big deal. Git releases are still readable.


Closes #496