Add pyproject.toml to maintain version from Kebechet's version manager #1163

shreekarSS · 2022-10-26T19:21:32Z

Signed-off-by: Shreekara sshreeka@redhat.com

Related Issues and Dependencies

Fixes: #1158

…

KPostOffice

Looks good! I'm worried about the length of adjust...

KPostOffice · 2022-10-28T17:19:47Z

kebechet/managers/version/release_triggers.py

@@ -108,6 +126,7 @@ def adjust_version_in_sources(
                    "version.py",
                    "app.py",
                    "wsgi.py",


WDYT about turning this into a list of tuples Tuple[str, function] where the function is based on how to process the file type. So, for all existing entries, the function would be self.adjust_version_file, but for the new entry it would self.adjust_version_toml_file or something like this? adjust_version_file is getting a bit long, so I think it should be either refactored into smaller functions, or the new functionality should go in its own function. Let me know what you think.

@KPostOffice I'm thinking about using dict of {file_name : function} , fetch the function based on file_name and call

harshad16

Good approach.

kebechet/managers/version/release_triggers.py

harshad16

some more suggestions

kebechet/managers/version/messages.py

kebechet/managers/version/release_triggers.py

harshad16

/lgtm
PTAL @KPostOffice

harshad16

One more note
update the docs as well
https://github.com/thoth-station/kebechet/blob/master/docs/managers/version.rst

VannTen · 2022-11-02T09:15:17Z

I have a question:
How do we handle version specified not in by the [project] section, but provided by the build backend ?
I'm thinking specifically of setup like:

[project]
dynamic = ["version"]

[tool.setuptools-scm]
enabled = true

or

[project]
dynamic = ["version"]

[tool.setuptools.dynamic]
version = {attr = "thoth.document_sync.__version__"}

(I'm not sure it's possible in a general way, but in that case we might want to state that in the docs)

harshad16 · 2022-11-02T19:12:51Z

/hold

harshad16 · 2022-11-02T19:15:57Z

That is a valid point, we should check/address if the version is not in the project section.

kebechet/managers/version/messages.py

harshad16

/lgtm

PTAL @KPostOffice

KPostOffice · 2022-11-03T20:57:16Z

python-poetry/poetry#144

An interesting thread about why "version" should never be put in source code and should instead be derived from package metadata when needed.

The basic argument here is that version should be set from package metadata, not the other way around.

KPostOffice · 2022-11-04T21:05:02Z

I'm not sure what this should mean for this PR, just a rabbit hole that I fell down when looking into Max's concern.

One way to just fix the issue raised is to recognize that json if version is a dictionary then it won't hold a proper string and to return early. If it is specified in a different file then the version manager should update it automatically already so throwing an exception would be wrong in this case.

harshad16 · 2022-11-07T17:36:42Z

I believe we are addressing the required pattern with PR.
we are only updating the pyproject.toml version if it is the project section, or else it wouldn't be picked for updated.
and we are describing this in our docs as well.

KPostOffice · 2022-11-08T20:21:08Z

I believe we are addressing the required pattern with PR. we are only updating the pyproject.toml version if it is the project section, or else it wouldn't be picked for updated. and we are describing this in our docs as well.

Oh yeah, I see what you mean

KPostOffice

Looks good. Just a few small changes to the new function

KPostOffice · 2022-11-08T21:50:35Z

kebechet/managers/version/release_triggers.py

+        project = toml_content.get("project", None)
+        old_version = project.get("version", None)
+        try:
+            new_version = self.get_new_version(old_version)


Old version must be a string. Passing None here will throw an error.

kebechet/managers/version/release_triggers.py

KPostOffice · 2022-11-08T21:53:05Z

kebechet/managers/version/release_triggers.py

+        if old_version:
+            toml_content["project"]["version"] = new_version
+            changed = True
+        if not changed:


Having changed and this conditional will be unnecessary if old_version is checked to not be None above

KPostOffice

Just a couple of small comments and then it should be good I think

KPostOffice · 2022-11-14T20:01:30Z

kebechet/managers/version/release_triggers.py

+        toml_content["project"]["version"] = new_version
+        new_content = toml.dumps(toml_content)
+        with open(file_path, "w") as output_file:
+            output_file.write(new_content)


Here you can directly do toml.dump(toml_content, output_file), no need for an intermediate string representation.

KPostOffice · 2022-11-14T20:07:00Z

kebechet/managers/version/release_triggers.py

-                    file_path = os.path.join(root, file_name)
-                    adjusted_version = self.adjust_version_file(file_path)
+                if file_name in files_dict:
+                    func_to_call: Any = files_dict[file_name]


could use Callable[[str], Optional[Tuple[str, str]]] as a type hint here. If it gives any errors, we can just leave it as callable.

Pre commit failed with : error: Incompatible types in assignment (expression has type "Callable[[str], Optional[Tuple[Any, ...]]]", variable has type "Callable[[str], Optional[Tuple[str, str]]]")

That's because of the function signatures of the called functions could be more descriptive too, so you would need to change this line as well as the the function signatures.

shreekarSS · 2022-11-14T21:09:10Z

/test pre-commit

KPostOffice · 2022-11-15T14:03:04Z

/retest

KPostOffice · 2022-11-15T22:04:11Z

kebechet/managers/version/release_triggers.py

+                        environment_details=ManagerBase.get_environment_details(),
+                    )
+                ) from e
+        _LOGGER.info("Computed new version: %s", new_version)


These need to be shifted over so that they are inside the conditional I'm pretty sure.

KPostOffice · 2022-11-15T22:13:06Z

kebechet/managers/version/release_triggers.py

+            toml_content["project"]["version"] = new_version
+            with open(file_path, "w") as output_file:
+                output_file.write(toml.dumps(toml_content))
+        return new_version, old_version


This also needs to be tabbed over. If there is no version, it should just return None

…ager - Update Messages ISSUE_BODY_NO_VERSION_IDENTIFIER_FOUND variable with pyproject.toml related Signed-off-by: Shreekara <sshreeka@redhat.com>

Signed-off-by: Shreekara <sshreeka@redhat.com>

KPostOffice · 2022-11-16T19:01:02Z

/approve

sesheta · 2022-11-16T19:01:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KPostOffice

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KPostOffice]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

harshad16

/unhold
/lgtm

thanks 👍

sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022

sesheta requested review from harshad16 and KPostOffice October 26, 2022 19:21

sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 26, 2022

shreekarSS force-pushed the add_pyproject.toml branch from 4d50815 to bd71123 Compare October 28, 2022 16:35

sesheta added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 28, 2022

shreekarSS force-pushed the add_pyproject.toml branch from bd71123 to e01a54c Compare October 28, 2022 16:36

shreekarSS changed the title ~~[WIP] Add pyproject.toml to maintain version from Kebechet's version manager~~ Add pyproject.toml to maintain version from Kebechet's version manager Oct 28, 2022

sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2022

KPostOffice reviewed Oct 28, 2022

View reviewed changes

shreekarSS force-pushed the add_pyproject.toml branch from e01a54c to 7f5f697 Compare November 1, 2022 17:31

sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2022

shreekarSS force-pushed the add_pyproject.toml branch from 7f5f697 to 12d3894 Compare November 1, 2022 17:33

harshad16 requested changes Nov 1, 2022

View reviewed changes

kebechet/managers/version/release_triggers.py Outdated Show resolved Hide resolved

shreekarSS force-pushed the add_pyproject.toml branch from 12d3894 to d7dc82a Compare November 1, 2022 17:48

harshad16 requested changes Nov 1, 2022

View reviewed changes

kebechet/managers/version/messages.py Outdated Show resolved Hide resolved

kebechet/managers/version/release_triggers.py Outdated Show resolved Hide resolved

shreekarSS force-pushed the add_pyproject.toml branch 2 times, most recently from ae216ee to cd1b7c1 Compare November 1, 2022 20:48

harshad16 reviewed Nov 1, 2022

View reviewed changes

sesheta assigned harshad16 Nov 1, 2022

sesheta added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2022

harshad16 reviewed Nov 1, 2022

View reviewed changes

sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2022

harshad16 reviewed Nov 2, 2022

View reviewed changes

kebechet/managers/version/messages.py Outdated Show resolved Hide resolved

shreekarSS force-pushed the add_pyproject.toml branch from cd1b7c1 to 78647ce Compare November 2, 2022 21:16

shreekarSS force-pushed the add_pyproject.toml branch 2 times, most recently from 96e77a1 to bc30c61 Compare November 3, 2022 14:44

harshad16 reviewed Nov 3, 2022

View reviewed changes

sesheta added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2022

KPostOffice reviewed Nov 8, 2022

View reviewed changes

shreekarSS force-pushed the add_pyproject.toml branch from bc30c61 to 738e43b Compare November 14, 2022 16:40

sesheta removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2022

KPostOffice reviewed Nov 14, 2022

View reviewed changes

shreekarSS force-pushed the add_pyproject.toml branch from 738e43b to 6f2a756 Compare November 14, 2022 21:06

shreekarSS force-pushed the add_pyproject.toml branch from e40cc27 to 9832eb7 Compare November 15, 2022 21:42

KPostOffice reviewed Nov 15, 2022

View reviewed changes

shreekarSS force-pushed the add_pyproject.toml branch from 9832eb7 to f525968 Compare November 15, 2022 22:09

KPostOffice reviewed Nov 15, 2022

View reviewed changes

shreekarSS added 2 commits November 16, 2022 11:40

- Add pyproject.toml to maintain version from Kebechet's version man…

baaf8ea

…ager - Update Messages ISSUE_BODY_NO_VERSION_IDENTIFIER_FOUND variable with pyproject.toml related Signed-off-by: Shreekara <sshreeka@redhat.com>

Update repo

4c2564f

Signed-off-by: Shreekara <sshreeka@redhat.com>

shreekarSS force-pushed the add_pyproject.toml branch from f525968 to 4c2564f Compare November 16, 2022 16:40

sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2022

harshad16 reviewed Nov 16, 2022

View reviewed changes

sesheta added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 16, 2022

sesheta merged commit f9f7f6d into thoth-station:master Nov 16, 2022

harshad16 mentioned this pull request Dec 5, 2022

sprint production release 2022.12.05 thoth-station/thoth-application#2700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pyproject.toml to maintain version from Kebechet's version manager #1163

Add pyproject.toml to maintain version from Kebechet's version manager #1163

shreekarSS commented Oct 26, 2022

KPostOffice left a comment

KPostOffice Oct 28, 2022

shreekarSS Nov 1, 2022

harshad16 left a comment

harshad16 left a comment

harshad16 left a comment

harshad16 left a comment

VannTen commented Nov 2, 2022

harshad16 commented Nov 2, 2022

harshad16 commented Nov 2, 2022

harshad16 left a comment

KPostOffice commented Nov 3, 2022

KPostOffice commented Nov 4, 2022

harshad16 commented Nov 7, 2022

KPostOffice commented Nov 8, 2022

KPostOffice left a comment

KPostOffice Nov 8, 2022

KPostOffice Nov 8, 2022

shreekarSS Nov 14, 2022

KPostOffice left a comment

KPostOffice Nov 14, 2022

shreekarSS Nov 14, 2022

KPostOffice Nov 14, 2022

shreekarSS Nov 14, 2022

KPostOffice Nov 15, 2022

shreekarSS commented Nov 14, 2022

KPostOffice commented Nov 15, 2022

KPostOffice Nov 15, 2022

shreekarSS Nov 15, 2022

KPostOffice Nov 15, 2022

shreekarSS Nov 16, 2022

KPostOffice commented Nov 16, 2022

sesheta commented Nov 16, 2022

harshad16 left a comment

Add pyproject.toml to maintain version from Kebechet's version manager #1163

Add pyproject.toml to maintain version from Kebechet's version manager #1163

Conversation

shreekarSS commented Oct 26, 2022

Related Issues and Dependencies

KPostOffice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

VannTen commented Nov 2, 2022

harshad16 commented Nov 2, 2022

harshad16 commented Nov 2, 2022

harshad16 left a comment

Choose a reason for hiding this comment

KPostOffice commented Nov 3, 2022

KPostOffice commented Nov 4, 2022

harshad16 commented Nov 7, 2022

KPostOffice commented Nov 8, 2022

KPostOffice 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

KPostOffice 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

shreekarSS commented Nov 14, 2022

KPostOffice commented Nov 15, 2022

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

KPostOffice commented Nov 16, 2022

sesheta commented Nov 16, 2022

harshad16 left a comment

Choose a reason for hiding this comment