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

[fix] Do not modify the settings too soon in change_url #376

Closed
wants to merge 1 commit into from

Conversation

maniackcrudelis
Copy link
Contributor

@maniackcrudelis maniackcrudelis commented Sep 28, 2017

Problems

If the domain is stored too soon, the backup script, used before any modification, fail because it loads the new domain instead of the old one.
The domain, and the path should be changed only at the end of the script.

Solution

Can't we use the new domain_url_available function instead ?

PR Status

Not tested.
Only a proposition.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

## Problems
If the domain is stored too soon, the backup script, used before any modification, fail because is loads the new domain instead of the old one.
The domain, and the path should be changed only at the end of the script.

## Solution
Can't we use the new `domain_url_available` function instead ?

## PR Status
Not tested.
Only a proposition.

## Validation

- [ ] Principle agreement 0/2 : 
- [ ] Quick review 0/1 : 
- [ ] Simple test 0/1 : 
- [ ] Deep review 0/1 :
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Not tested, but that sounds very LGTM ;)

@Psycojoker
Copy link
Member

I'm wondering if we effectively save the domain after that then, here we just check it.

Otherwise no opposition to that since it's coming from the people best informed about that subject.

@alexAubin
Copy link
Member

Ah indeed, as pointed by @Psycojoker , it should probably be more like :

if not domain_url_available(...) :
    # Throw 
    raise MoulinetteException(...)

domain_url_register(...)

(To be tested / implemented)

@alexAubin
Copy link
Member

(Flagging as "work needed" for now, because as said we need more than just checking the availability.)

@alexAubin
Copy link
Member

Closing because superseded by #608

@alexAubin alexAubin closed this Jan 16, 2019
@alexAubin alexAubin deleted the change_url_store_settings branch January 16, 2019 23:52
@alexAubin alexAubin removed this from the 3.x milestone Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants