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

Fix wrong wording #85

Closed
wants to merge 1 commit into from
Closed

Fix wrong wording #85

wants to merge 1 commit into from

Conversation

delgod
Copy link
Member

@delgod delgod commented Jan 26, 2023

Issue

Solution

Context

Testing

Release Notes

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines -141 to +142
master_start_timeout = configuration_info.json().get("master_start_timeout")
return int(master_start_timeout) if master_start_timeout is not None else None
primary_start_timeout = configuration_info.json().get("primary_start_timeout")
return int(primary_start_timeout) if primary_start_timeout is not None else None
Copy link
Member

Choose a reason for hiding this comment

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

This depends on patroni/patroni#2541 (and later on updating the Patroni version in this repo).

Comment on lines +126 to +127
async def get_primary_start_timeout(ops_test: OpsTest) -> Optional[int]:
"""Get the primary start timeout configuration.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to update the name when the configuration name is changed on Patroni (patroni/patroni#2541).

@@ -48,12 +48,12 @@ async def app_name(ops_test: OpsTest, application_name: str = "postgresql-k8s")
return None


async def change_master_start_timeout(ops_test: OpsTest, seconds: Optional[int]) -> None:
"""Change master start timeout configuration.
async def change_primary_start_timeout(ops_test: OpsTest, seconds: Optional[int]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to update the name when the configuration name is changed on Patroni (patroni/patroni#2541).

@@ -62,7 +62,7 @@ async def change_master_start_timeout(ops_test: OpsTest, seconds: Optional[int])
unit_ip = await get_unit_address(ops_test, primary_name)
requests.patch(
f"http://{unit_ip}:8008/config",
json={"master_start_timeout": seconds},
json={"primary_start_timeout": seconds},
Copy link
Member

Choose a reason for hiding this comment

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

This depends on patroni/patroni#2541.

@@ -2,7 +2,7 @@ kind: Service
apiVersion: v1
metadata:
labels:
role: master
role: primary
Copy link
Member

Choose a reason for hiding this comment

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

This will need more work on Patroni even after patroni/patroni#2541 is merged and published, as per patroni/patroni#2541 (comment).

Comment on lines -152 to +155
postmaster_start_time = datetime.strptime(
health_info["postmaster_start_time"], "%Y-%m-%d %H:%M:%S.%f%z"
postprimary_start_time = datetime.strptime(
health_info["postprimary_start_time"], "%Y-%m-%d %H:%M:%S.%f%z"
).timestamp()
return postmaster_start_time > restart_time and health_info["state"] == "running"
return postprimary_start_time > restart_time and health_info["state"] == "running"
Copy link
Member

Choose a reason for hiding this comment

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

postmaster is a deprecated alias to postgres since PostgreSQL 8.2, but it still has a function called pg_postmaster_start_time, which is used by Patroni. However, we can change the name of our variable.

"master": primary,
"primary": primary,
Copy link
Member

Choose a reason for hiding this comment

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

This probably will break legacy relations, as charms expect this field in the relation.

return "master"
return "primary"
Copy link
Member

Choose a reason for hiding this comment

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

This probably will break legacy relations, as charms expect this value in this specific field from the relation.

@taurus-forever
Copy link
Contributor

This PR cannot be merged AS IS and requires changes on Patroni first.
IMHO, to keep the house clean, @marceloneppel please create an GitHub->Jira issue to replace master->primary when it will be technically possible. With this PR can be abandoned.

@marceloneppel
Copy link
Member

This PR cannot be merged AS IS and requires changes on Patroni first. IMHO, to keep the house clean, @marceloneppel please create an GitHub->Jira issue to replace master->primary when it will be technically possible. With this PR can be abandoned.

Done. Created #101.

github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-k8s-operator that referenced this pull request Jun 20, 2024
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-k8s-operator that referenced this pull request Jun 20, 2024
github-actions bot added a commit to canonical/test-runners-2-is-arm64-postgresql-k8s-operator that referenced this pull request Jun 21, 2024
github-actions bot added a commit to canonical/test-runners-2-azure-arm64-postgresql-k8s-operator that referenced this pull request Jul 24, 2024
@delgod delgod deleted the fix-wording branch July 30, 2024 09:24
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