Update BTR release scripts to look at catalog-info.yaml for release info.#439 #467

salman2013 · 2023-12-14T11:32:45Z

Description:

In this PR i updated the BTR release script which was previously checking openedx.yaml file to find the openedx-release tag and performing other release preparation operations, now the updated script first check the catalog_info.yaml file in all repositories and in case that file is not present it checks for openedx.yaml. I also tried to update some test cases related to it.

Ticket: https://github.com/orgs/openedx/projects/55/views/1?pane=issue&itemId=37975469

nedbat

I didn't look through the tests yet, because the basic logic is different than I expected.

nedbat · 2023-12-15T17:44:22Z

edx_repo_tools/release/tag_release.py

-        if data.get('openedx-release'):
-            repo = repo.refresh()
-            repos[repo] = data
+    repos = catalog_info_file(hub, orgs=orgs, branches=branches)


This looks like it will find all repos with a catalog-info.yaml file, and if it finds any, it won't look for repos with openedx.yaml. Is that what you want?

I thought the idea was the check each repo for a catalog-info.yaml file, and if the file isn't there, check for an openedx.yaml file.

Ned, you're right, it should look at each repo individually as you said. @salman2013 can you fix the code so it does that?

@nedbat @feanil my bad i misunderstood the requirements, let me update it and come back with another review.

@nedbat @feanil I fixed the code logic as per requirement, could you please take an other look, thanks.

…r openedx release

OmarIthawi · 2024-01-09T06:14:55Z

@salman2013 I'm jumping here to check about the openedx-translations catalog-info.yaml file.

That repo wasn't included in Olive because it had no openedx.yaml file.

Would this update include it for Redwood branches and tags?

feanil · 2024-01-10T18:20:00Z

@OmarIthawi after we merged this, we'll still need to add a new annotation to existing catalog-info.yaml files before they are included.

feanil

@salman2013 I think this is more on the right track but not quite right yet. In the current version, if we have a catalog-info.yaml at all, it will ignore the openedx.yaml file entirely. This is not exactly what we want, we want it to specifically ignore the openedx.yaml file only if there is data related to the release in the catalog-info.yaml

Rather than changing the current code that reads the openedx.yaml file, I suggest you just add new code that reads the catalog-info.yaml file and then pass the data from both up to the tag_release.py script which can then be smarter about what data it looks at.

feanil · 2024-01-12T20:47:29Z

edx_repo_tools/data.py

+def find_file_content(repo, branch):
    """
-    Yield the data from all openedx.yaml files found in repositories in ``orgs``
+    Finds the data from all catalog-info.yaml or openedx.yaml files found in repositories in ``repo``
+    on any of ``branches``.
+    First, we check for catalog-info.yaml, If catalog-info.yaml is not found, we look for openedx.yaml file.
+
+    """


The function signature and code suggest that it is a single repo and branch but the docstring seems to be referring to multiple repos and branches, please update the docstring.

I removed this method when resolving the review comments and code logic.

feanil · 2024-01-12T20:48:04Z

edx_repo_tools/data.py

+            contents = repo.file_contents(file_name, ref=branch)
+            break
+        except NotFoundError:
+            contents = None


nit: You don't need to re-assign this to none, since that's already the value, you can just call pass when catching the exception.

I removed this method when resolving the review comments and code logic.

feanil · 2024-01-12T20:49:29Z

edx_repo_tools/data.py


            if contents is not None:
-                LOGGER.debug("Found openedx.yaml at %s:%s", repo.full_name, branch)
+                LOGGER.debug("Found %s at %s:%s", contents, repo.full_name, branch)


We don't want to print the whole of contents here, but the name of the file it found. It maybe worth updating the find_file_contents code to also return the name of the file that it got the info from.

I fixed it.

feanil · 2024-01-12T20:50:06Z

edx_repo_tools/data.py

                try:
                    data = yaml.safe_load(contents.decoded)
                except Exception as exc:
-                    LOGGER.error("Couldn't parse openedx.yaml from %s:%s, skipping repo", repo.full_name, branch, exc_info=True)
+                    LOGGER.error("Couldn't parse %s from %s:%s, skipping repo", contents, repo.full_name, branch, exc_info=True)


Same here, it should be the name of the file, not the full contents as that will be a lot of yaml to print and make it harder to read and understand the messages.

I fixed it.

feanil · 2024-01-12T20:51:34Z

edx_repo_tools/oep2/explode_repos_yaml.py

    """
    data = {
        repo.full_name: openedx_yaml
        for repo, openedx_yaml
-        in iter_openedx_yaml(hub, org, branch)
+        in iter_openedx_release_yaml(hub, org, branch)


Hmm, it may not make sense to update this tool at all. This one is specifically for reading the openedx.yaml and shouldn't mix it with catalog-info.yaml.

I fixed it.

salman2013 · 2024-01-24T11:46:16Z

@feanil I have updated the PR and its ready for another round of review.

feanil · 2024-02-02T19:27:48Z

edx_repo_tools/oep2/explode_repos_yaml.py

@@ -8,7 +8,7 @@
 import yaml

 from edx_repo_tools.auth import pass_github
-from edx_repo_tools.data import iter_openedx_yaml
+from edx_repo_tools.data import iter_openedx_release_yaml


You no longer renamed this function but you did change the function signature, I think you need to update calls to this function.

I missed that by mistake, now fixed it. thanks.

arbrandes · 2024-02-27T18:06:47Z

FYI @cmltaWt0 as current release manager. This should be transparent for future releases, but still good to be aware of.

feanil

Generally looks good to me and I've tested locally. I found one issue which I made a PR against your PR for: salman2013#1

chore: update btr tag_release script

03d61ea

salman2013 requested review from feanil, farhan and nedbat December 14, 2023 11:34

salman2013 mentioned this pull request Dec 14, 2023

Update BTR release scripts to look at catalog-info.yaml for release info. #439

Closed

2 tasks

salman2013 self-assigned this Dec 14, 2023

nedbat reviewed Dec 15, 2023

View reviewed changes

chore: fix the logic for looking into catalog-info or openedx file fo…

646e705

…r openedx release

salman2013 requested a review from nedbat December 18, 2023 14:41

feanil reviewed Jan 12, 2024

View reviewed changes

chore: fix review comments and improve code

3abf653

salman2013 requested a review from feanil January 30, 2024 10:44

feanil reviewed Feb 2, 2024

View reviewed changes

chore: fix the import method name

bfb9b88

salman2013 requested a review from feanil February 6, 2024 11:53

feanil mentioned this pull request Feb 13, 2024

Use catalog-info.yaml for release metadata. Get rid of openedx.yaml openedx/open-edx-proposals#526

Merged

feanil reviewed Mar 5, 2024

View reviewed changes

feanil and others added 2 commits March 6, 2024 21:41

fix: The annotations section can exist and be empty. (#1)

ca2f97e

Merge branch 'master' into salman/update-btr-release-script

826ec29

feanil approved these changes Mar 6, 2024

View reviewed changes

feanil merged commit e24267e into openedx:master Mar 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update BTR release scripts to look at catalog-info.yaml for release info.#439 #467

Update BTR release scripts to look at catalog-info.yaml for release info.#439 #467

salman2013 commented Dec 14, 2023 •

edited

Loading

nedbat left a comment

nedbat Dec 15, 2023

feanil Dec 15, 2023

salman2013 Dec 15, 2023

salman2013 Dec 18, 2023

OmarIthawi commented Jan 9, 2024

feanil commented Jan 10, 2024

feanil left a comment

feanil Jan 12, 2024

salman2013 Jan 24, 2024

feanil Jan 12, 2024

salman2013 Jan 24, 2024

feanil Jan 12, 2024

salman2013 Jan 24, 2024

feanil Jan 12, 2024

salman2013 Jan 24, 2024

feanil Jan 12, 2024

salman2013 Jan 24, 2024

salman2013 commented Jan 24, 2024

feanil Feb 2, 2024

salman2013 Feb 6, 2024

arbrandes commented Feb 27, 2024

feanil left a comment

Update BTR release scripts to look at catalog-info.yaml for release info.#439 #467

Update BTR release scripts to look at catalog-info.yaml for release info.#439 #467

Conversation

salman2013 commented Dec 14, 2023 • edited Loading

nedbat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Jan 9, 2024

feanil commented Jan 10, 2024

feanil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salman2013 commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrandes commented Feb 27, 2024

feanil left a comment

Choose a reason for hiding this comment

salman2013 commented Dec 14, 2023 •

edited

Loading