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

Make the STABLE and LATEST constants overridable #4099

Merged
merged 1 commit into from Sep 4, 2018

Conversation

Projects
None yet
6 participants
@xrmx
Contributor

xrmx commented May 16, 2018

Moving them to settings and making life easier for downstreams
that want to use different labels.

@ericholscher

This looks useful. I definitely want to maintain backwards compat though. If the goal is to simply make it able to be changed with a setting, a much less invasive change would be to have the constants be able to read from a setting, instead of changing every instance of it.

Show outdated Hide outdated readthedocs/builds/constants.py Outdated
@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx May 29, 2018

Contributor

We are currently shipping the following commit:
italia@fc50de4

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

Contributor

xrmx commented May 29, 2018

We are currently shipping the following commit:
italia@fc50de4

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

@agjohnson agjohnson added this to the Refactoring milestone Jun 8, 2018

@agjohnson

Also echoing the need to move this back to constants for downstream.

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jun 8, 2018

Contributor

Do you have any idea on how to make them configurable but still keep them in constants? I don't.

Contributor

xrmx commented Jun 8, 2018

Do you have any idea on how to make them configurable but still keep them in constants? I don't.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Jun 18, 2018

Member

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

@xrmx can you talk more about the reasons that italia@fc50de4 won't work?

I don't think we're ready to commit to this PR's approach in all our downstream dependencies, but I could perhaps be convinced if shown why the simpler settings-based approach can't work.

Member

ericholscher commented Jun 18, 2018

Downside of this is that we cannot ship the two together IIRC because of circular dependencies and tests still has them hardcoded so can't test out of the box with a different value in settings.

@xrmx can you talk more about the reasons that italia@fc50de4 won't work?

I don't think we're ready to commit to this PR's approach in all our downstream dependencies, but I could perhaps be convinced if shown why the simpler settings-based approach can't work.

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jun 18, 2018

Contributor

@ericholscher it will work on its own and we are currently using it. But i think that using plain settings for something that should be configurable would be cleaner. Anyway if you are fine with that approach I can update the PR. The smaller delta we have from upstream the better.

Contributor

xrmx commented Jun 18, 2018

@ericholscher it will work on its own and we are currently using it. But i think that using plain settings for something that should be configurable would be cleaner. Anyway if you are fine with that approach I can update the PR. The smaller delta we have from upstream the better.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Jun 18, 2018

Member

Yea, I think that is a much easier change to make at least for now. We can talk about the larger one later.

Member

ericholscher commented Jun 18, 2018

Yea, I think that is a much easier change to make at least for now. We can talk about the larger one later.

builds: make latest and stable constants overridable
Makes the constants overridable from settings so that downstream
can use different ones.
@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jun 19, 2018

Contributor

Updated PR as agreed.

Contributor

xrmx commented Jun 19, 2018

Updated PR as agreed.

@ericholscher

This looks like a good change. Curious @xrmx what are y'all using for these versions instead of latest and stable?

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jun 19, 2018

Contributor

@ericholscher roughly the same but in italian :) draft instead of latest and stable is the same.

Contributor

xrmx commented Jun 19, 2018

@ericholscher roughly the same but in italian :) draft instead of latest and stable is the same.

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jul 4, 2018

Contributor

@agjohnson care to update review after different proposed implementation please? Thanks!

Contributor

xrmx commented Jul 4, 2018

@agjohnson care to update review after different proposed implementation please? Thanks!

@humitos

humitos approved these changes Jul 5, 2018

Thanks!

The new approach it's better to me. I think we are safe to merge this PR.

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Jul 18, 2018

Contributor

ping, we have a couple positive reviews, can we merge it please?

Contributor

xrmx commented Jul 18, 2018

ping, we have a couple positive reviews, can we merge it please?

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 16, 2018

Member

This is ready to merge. I'm assigning this to myself to merge after the migration that it's taking place this weekend.

Member

humitos commented Aug 16, 2018

This is ready to merge. I'm assigning this to myself to merge after the migration that it's taking place this weekend.

@humitos humitos self-assigned this Aug 16, 2018

@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Sep 3, 2018

Contributor

@humitos any update?

Contributor

xrmx commented Sep 3, 2018

@humitos any update?

@humitos

humitos approved these changes Sep 4, 2018

Thanks!

@humitos humitos merged commit bde0088 into rtfd:master Sep 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xrmx

This comment has been minimized.

Show comment
Hide comment
@xrmx

xrmx Sep 5, 2018

Contributor

@humitos thanks!

Contributor

xrmx commented Sep 5, 2018

@humitos thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment