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

One migration pending in 1.1.0 #400

Closed
wants to merge 1 commit into from
Closed

One migration pending in 1.1.0 #400

wants to merge 1 commit into from

Conversation

truizsanchez
Copy link

Migrations doesn't reflect the current Request#prof_file attribute, so one migration is still pending to be generated. That difficulties working with the library in some CI and automated environments

Migrations doesn't reflect the current Request#prof_file attribute, so one migration is still pending to be generated. That difficulties working with the library in some CI and automated environments
@truizsanchez truizsanchez changed the title Add files via upload One migration pending in 1.1.0 Feb 17, 2020
@nasirhjafri
Copy link
Member

Isn't 1.1.0 a very old release?

@nasirhjafri nasirhjafri self-requested a review February 17, 2020 08:09
@truizsanchez
Copy link
Author

Isn't 1.1.0 a very old release?

Hehe I know, but we have a legacy environment with django 1.7 and python 2.7...

@nasirhjafri
Copy link
Member

Isn't 1.1.0 a very old release?

Hehe I know, but we have a legacy environment with django 1.7 and python 2.7...

This migration already exists in 1.1.0 https://github.com/jazzband/django-silk/blob/c9438044e59ae20da2406958b043a0f7fdce67e8/silk/migrations/0005_increase_request_prof_file_length.py

@truizsanchez
Copy link
Author

Isn't 1.1.0 a very old release?

Hehe I know, but we have a legacy environment with django 1.7 and python 2.7...

This migration already exists in 1.1.0 https://github.com/jazzband/django-silk/blob/c9438044e59ae20da2406958b043a0f7fdce67e8/silk/migrations/0005_increase_request_prof_file_length.py

Yes, migration #5 is changing Request#prof_file attribute, but the problem is with storage attribute, is not evaluated in previous migrations, so in migration #6 is taking the default value:
defaults = {
...
'SILKY_STORAGE_CLASS': 'silk.storage.ProfilerResultStorage'
}

I do not know if you want to always have previously made makemigrations before using the library, but if that is the case it makes it difficult to work in certain automated environments, requiring additional steps to have the library ready to use.

Any criticism is welcome, I hope to be useful :)

@nasirhjafri
Copy link
Member

Test cases are failing, please fix them. Even if we want to merge this one we can't backport it to 1.1.0.
That being said it would only be available in 4.0.1. I'll test it once the tests are passed.

@nasirhjafri
Copy link
Member

Isn't 1.1.0 a very old release?

Hehe I know, but we have a legacy environment with django 1.7 and python 2.7...

This migration already exists in 1.1.0 https://github.com/jazzband/django-silk/blob/c9438044e59ae20da2406958b043a0f7fdce67e8/silk/migrations/0005_increase_request_prof_file_length.py

Yes, migration #5 is changing Request#prof_file attribute, but the problem is with storage attribute, is not evaluated in previous migrations, so in migration #6 is taking the default value:
defaults = {
...
'SILKY_STORAGE_CLASS': 'silk.storage.ProfilerResultStorage'
}

I do not know if you want to always have previously made makemigrations before using the library, but if that is the case it makes it difficult to work in certain automated environments, requiring additional steps to have the library ready to use.

Any criticism is welcome, I hope to be useful :)

So It was fixed later in v2.0.0 5ba0853

If you want to use it with v1.1.0 Please take a fork from v1.1.0 release tag and cherry-pick this commit on top of it 5ba0853

@truizsanchez
Copy link
Author

Thank you, I made a mistake with branches bringing master...

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