[go: nahoru, domu]

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

feat(universe): enable targetting hosting environment dynamically #138

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

eljefedelrodeodeljefe
Copy link
Contributor
@eljefedelrodeodeljefe eljefedelrodeodeljefe commented Mar 16, 2022

Signed-off-by: Robert Jefe Lindstaedt robert.lindstaedt@gmail.com

it can easily break stuff in the agent UI once I would merge there. Potentially I will break the dev build temporarily, but I do not see a way around it.

We will need it for hosting universes e.g. at charlesuniversesdev.com

Signed-off-by: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Copy link
Collaborator
@artdnlv artdnlv left a comment

Choose a reason for hiding this comment

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

LGTM

super({
injectables: {
// symmetric to the legacy implementation below
base: options.universeBase ?? `https://${options.name}.hello-charles.com`
base: options.universeBase ?? `https://${options.name}.${options.universeHost ?? 'hello-charles.com'}`
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 suggest to move this hello-charles.com to some const DEFAULT_UNIVERSE_HOST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not work because of the TS compiler unfortunately, see ref in code or microsoft/TypeScript#8277

Copy link
Contributor
@haayhappen haayhappen left a comment

Choose a reason for hiding this comment

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

LGTM, why do you think it would break though?

@eljefedelrodeodeljefe
Copy link
Contributor Author

LGTM, why do you think it would break though?

Because the we have a lot of branching in code now, which is not desired, and makes it a bit intransparent what host we are targeting. I believe the test though to be exhaustive and hope that the integration in the agent UI is represented by that. PR there following

@eljefedelrodeodeljefe eljefedelrodeodeljefe merged commit 87abae7 into master Mar 16, 2022
@eljefedelrodeodeljefe eljefedelrodeodeljefe deleted the feat/universe-set-hosting-environment branch March 16, 2022 13:53
@github-actions
Copy link

🎉 This PR is included in version 4.75.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants