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

Chore: housekeeping #3

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Chore: housekeeping #3

wants to merge 13 commits into from

Conversation

RedRoserade
Copy link
Collaborator

@RedRoserade RedRoserade commented May 8, 2022

This does a few changes, and should probably be split into multiple PRs for digestibility.

  • It moves away from aioredis to redis-py, because the former is deprecated and moved into the main package
  • It updates pinned dependencies (dependency requirements of the package didn't actually change apart from aioredis)
  • It applies isort for sorting imports. Because of the large diff, an "ignore revs" file has been created
  • It tries to be more local-development friendly by allowing the configuration to be set externally and by having a dedicated docker-compose.yaml file just for Redis and RabbitMQ
  • It adds a Makefile with common commands to format code, lint, and test
  • Update links to match the new org

Still want to implement:

  • MyPy for type checking
  • Update CI to run linting and testing automatically

@dolfim-ibm
Copy link
Collaborator

Looks great so far, but I get you are still working on it before we should all review it properly, right?

Copy link

@PeterStaar-IBM PeterStaar-IBM left a comment

Choose a reason for hiding this comment

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

nice clean-up!

@RedRoserade
Copy link
Collaborator Author

Looks great so far, but I get you are still working on it before we should all review it properly, right?

Yes, I still have some things I want to do. Should I split the PR into at least two? Maybe one that only has the Redis changes?

@dolfim-ibm
Copy link
Collaborator

Looks great so far, but I get you are still working on it before we should all review it properly, right?

Yes, I still have some things I want to do. Should I split the PR into at least two? Maybe one that only has the Redis changes?

Good idea, let's start to have that merged in independently

@RedRoserade RedRoserade force-pushed the chore-housekeeping branch 2 times, most recently from 92abe92 to 89a57df Compare July 6, 2022 19:54
@RedRoserade RedRoserade force-pushed the chore-housekeeping branch 4 times, most recently from f0a6058 to 610abe3 Compare July 17, 2022 18:28
Signed-off-by: André Carvalho <[email protected]>
@RedRoserade RedRoserade marked this pull request as ready for review July 17, 2022 18:49

- name: Test
# Wait for RabbitMQ to start.
run: sleep 10 && make test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like this; it would be nice for the tests to wait for RabbitMQ to actually be ready. Also, for some reason the tests didn't do anything for 10 minutes on 3 runs, so maybe this should be disabled?

Suggestions welcome.

@dolfim-ibm
Copy link
Collaborator

@RedRoserade do you understand why some tests take > 1h? Maybe the matrix strategy has some colliding host?

@RedRoserade
Copy link
Collaborator Author

I don't know. I saw builds yesterday hanging with no output on the test stage, and there's currently builds going on for 4 hours as of this comment? That's not normal at all, maybe the test and docker part should be turned off for now.

@dolfim-ibm
Copy link
Collaborator

That's not normal at all, maybe the test and docker part should be turned off for now.

I would propose to turn off the matrix, and see if testing only with ^3.8 will be more consistent

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