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

(#634) restore custom domain when recreating project #637

Merged
merged 2 commits into from
Feb 22, 2023
Merged

(#634) restore custom domain when recreating project #637

merged 2 commits into from
Feb 22, 2023

Conversation

angelorendina
Copy link
Contributor

@angelorendina angelorendina commented Feb 16, 2023

feat: restore previous custom domain (if any) when recreating a project (closes #634)

gateway/src/service.rs Show resolved Hide resolved
gateway/src/service.rs Show resolved Hide resolved
gateway/src/service.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@angelorendina angelorendina left a comment

Choose a reason for hiding this comment

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

Rebased and pushed more commits, also waiting to see if the CI is happy this time so still keeping this a draft

Comment on lines -204 to -207
pub fn create(project_name: ProjectName) -> Self {
Self::Creating(ProjectCreating::new_with_random_initial_key(project_name))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(3rd commit proposal, can be removed if not liked)
Removing this public method as there is already a builder pattern for the inner ProjectCreating.
Alternatively one could change this method to accept a fqdn: Option<String> as well, to reflect the current usage of the method - I prefer the former though.

@angelorendina angelorendina marked this pull request as ready for review February 19, 2023 13:49
Copy link
Contributor

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Angelo! I appreciate the test and the review comments explaining your reasoning!

…#634)

* test: ProjectCreating fqdn dev-only getter and completed unit test

* refactor: remove Project::create in favour of internal builder pattern
@angelorendina
Copy link
Contributor Author

@oddgrd thanks for the review. I have just rebased from latest master and squashed the commits into one, so it should be good to go as per your guidelines 👍

gateway/src/project.rs Outdated Show resolved Hide resolved
@chesedo
Copy link
Contributor

chesedo commented Feb 22, 2023

Thanks for this @angelorendina!

@chesedo chesedo merged commit 9d9035f into shuttle-hq:main Feb 22, 2023
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.

Recreating a project with a custom domain makes it lose it
3 participants