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

Pin python dependencies with Poetry #60

Merged
merged 13 commits into from
Aug 25, 2023
Merged

Pin python dependencies with Poetry #60

merged 13 commits into from
Aug 25, 2023

Conversation

carlcsaposs-canonical
Copy link
Contributor

@carlcsaposs-canonical
Copy link
Contributor Author

Depends on canonical/data-platform-workflows#80

@carlcsaposs-canonical
Copy link
Contributor Author

Note that tls and shared db integration tests were disabled before this PR

get_count_keystone_tables_sql,
)
assert output[0] > 0
# TODO: re-enable when bug resolved: https://bugs.launchpad.net/charm-keystone/+bug/1990243
Copy link
Contributor

Choose a reason for hiding this comment

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

i am unsure if commenting these tests is ideal - i would rather they be deleted if they are not running or mark them unstable. is there any reason they're being commented instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think they're unstable—just broken

The unstable mark runs the tests nightly

In order to maintain the same behavior in the repo as before the PR, the tests need to be commented out so a runner isn't provisioned in the collect_groups step (I previously added pytest.mark.skip(), but it provisioned a runner)

Should we delete them then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in favor of deleting them. Thoughts @paulomach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: https://bugs.launchpad.net/charm-keystone/+bug/1990243 is marked as "Will not fix" (as already fixed in another library). If the test is still broken, we need to reopen the ticket, otherwise re-enable the test. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer for that to be a separate ticket since the test was already disabled. For now, should I delete or leave commented out?

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 for the changes. Great work.
Your plans with tests are not clear for me.

get_count_keystone_tables_sql,
)
assert output[0] > 0
# TODO: re-enable when bug resolved: https://bugs.launchpad.net/charm-keystone/+bug/1990243
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: https://bugs.launchpad.net/charm-keystone/+bug/1990243 is marked as "Will not fix" (as already fixed in another library). If the test is still broken, we need to reopen the ticket, otherwise re-enable the test. Isn't it?

@carlcsaposs-canonical carlcsaposs-canonical merged commit 3bab177 into main Aug 25, 2023
9 of 10 checks passed
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