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.

Sign up for GitHub

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

Jump to bottom

Catch up poetry to 1.5 and fix path detection #1489

Merged
merged 5 commits into from
Oct 19, 2023
Merged

Catch up poetry to 1.5 and fix path detection #1489

merged 5 commits into from
Oct 19, 2023

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Oct 18, 2023 •
edited
Loading

Description of your changes:
This PR updates our code base to catch up with poetry to 1.5, fixes poetry path detection, removes old and unused install script and updates the dev readme.

Which issue is resolved by this Pull Request:
Resolves #1460 (comment)

Needs scylladb/scylla-operator-images#35 (+postsubmit) to pass CI.

Old postsubmits are replaced with https://github.com/scylladb/scylla-operator-release/pull/47 so we can converge on the same build style and version for presubmits and postsubmits.

@tnozicka tnozicka added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. labels Oct 18, 2023
@scylla-operator-bot scylla-operator-bot bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 18, 2023
@tnozicka tnozicka mentioned this pull request Oct 18, 2023
Merged
rzetelskik
rzetelskik reviewed Oct 18, 2023
View reviewed changes
docs/README-dev.md Outdated
Here is an example how you can start quickly using containers, similarly to how our CI runs it

```bash
$ podman run -it --pull=Always --rm -v="$( pwd )/docs:/go/$( go list -m )/docs:Z" --workdir="/go/$( go list -m )/docs" -p 5500:5500 quay.io/scylladb/scylla-operator-images:poetry-1.5 bash -euExo pipefail -O inherit_errexit -c 'poetry install && make multiversionpreview'
Copy link
Member

Choose a reason for hiding this comment

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

Let's delete the dollar sign so that it's easier to copy

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that say that you need to clone the repository anyway? Since it uses the makefile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's delete the dollar sign so that it's easier to copy

ok, guess I tend to be consistent about bash snippets and in the others, this differentiates the command from it's output.

Shouldn't that say that you need to clone the repository anyway?

I'd be hesitant to add something that obvious, given it also mounts a docs subdir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a sentence about being at repository root that should put this at ease

Copy link
Member

Choose a reason for hiding this comment

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

I'd be hesitant to add something that obvious, given it also mounts a docs subdir

Ok, it's for developers so I guess we can expect it to be obvious

docs/README-dev.md Outdated
## Update dependencies

```bash
$ podman run -it --pull=Always --rm -v="$( pwd )/docs:/go/$( go list -m )/docs:Z" --workdir="/go/$( go list -m )/docs" quay.io/scylladb/scylla-operator-images:poetry-1.5 bash -euExo pipefail -O inherit_errexit -c 'poetry update'
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

/hold
(I need to check the old post submits)

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023
zimnx
zimnx approved these changes Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023
@tnozicka tnozicka changed the title Catch up poetry to 1.5 and fix path detection [WIP] Catch up poetry to 1.5 and fix path detection Oct 18, 2023
@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2023
@scylla-operator-bot scylla-operator-bot bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
@tnozicka tnozicka changed the title [WIP] Catch up poetry to 1.5 and fix path detection Catch up poetry to 1.5 and fix path detection Oct 19, 2023
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2023
@tnozicka
Copy link
Contributor Author

/hold cancel
postsubmits are removed here and migrated in https://github.com/scylladb/scylla-operator-release/pull/47 so we unify on a single way to build docs

@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2023
zimnx
zimnx approved these changes Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

thanks

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
@scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tnozicka, zimnx

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:

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

@scylla-operator-bot scylla-operator-bot bot merged commit 5805b73 into scylladb:master Oct 19, 2023
@tnozicka tnozicka deleted the update-poetry branch October 19, 2023 13:03
@rzetelskik rzetelskik mentioned this pull request Nov 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rzetelskik rzetelskik rzetelskik left review comments

@zimnx zimnx zimnx approved these changes

Assignees

@zimnx zimnx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. lgtm Indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tnozicka @zimnx @rzetelskik