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

Remove pandas to make build working again #589

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

FlorianSW
Copy link
Collaborator

Upgrading to the latest version of pandas would've worked, too. However, the server stats API is not finnisshed, not run on regular users workloads (except they intentionally enabled it) and was most likely not finished at all. No need in keeping it for much longer.

rcon/models.py Outdated Show resolved Hide resolved
@@ -225,7 +225,6 @@ class Meta:
"Can view the amount of time left in the round",
),
("can_view_server_name", "Can view the server name"),
("can_view_server_stats", "Can view the get_server_stats endpoint"),
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 know enough about the permissions in Django tbh :( Do I need to do anything else here to make this change flawless? Like, will the permission automatically be removed from the groups and users that own them right now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably just leave the permissions in case we want to use them later, they don't really hurt anything.

But this models.py file needs to be updated and then we'd need to modify migration 2 or 3 (I forget) which creates the default groups

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'd probably just leave the permissions in case we want to use them later, they don't really hurt anything.

I don't think that this would be a very good thing. People might misinterpret the permission, especially given when it does not do anything :/

But this models.py file needs to be updated and then we'd need to modify migration 2 or 3 (I forget) which creates the default groups

👍

@FlorianSW FlorianSW requested a review from cemathey June 22, 2024 18:31
@cemathey
Copy link
Collaborator

This got resolved on master with a change to requirements.txt to pin numpy; do we want to keep this or close it?

@FlorianSW
Copy link
Collaborator Author

FlorianSW commented Aug 26, 2024

I mean, pandas is a big dependency, not sure how useful it is, if we don't actually use it 😅
-> I would still remove it

@cemathey
Copy link
Collaborator

cemathey commented Sep 9, 2024

I agree I think we can remove this stuff; has merge conflicts now though 🤔

@FlorianSW
Copy link
Collaborator Author

I'll look into them after my event I play today 👍

Upgrading to the latest version of pandas would've worked, too.
However, the server stats API is not finnisshed, not run on regular
users workloads (except they intentionally enabled it) and was most
likely not finished at all. No need in keeping it for much longer.
@FlorianSW
Copy link
Collaborator Author

@cemathey rebased the PR, could you take another look? ❤️

Copy link
Collaborator

@cemathey cemathey left a comment

Choose a reason for hiding this comment

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

lgtm and built and launched without issue

@cemathey cemathey merged commit 6a99f16 into MarechJ:master Oct 24, 2024
@FlorianSW FlorianSW deleted the maint/pandas branch October 24, 2024 16:07
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.

2 participants