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

Adding expose application #3215

Merged
merged 20 commits into from
Aug 28, 2024
Merged

Conversation

maayarosama
Copy link
Contributor

@maayarosama maayarosama commented Aug 4, 2024

Description

Adding Domains solution to the dashboard
Screenshot from 2024-08-18 15-15-19
Screenshot from 2024-08-18 15-27-42

Related Issues

Documentation PR

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

@Mik-TF
Copy link
Contributor

Mik-TF commented Aug 7, 2024

Note: I adjusted 2 basic texts on expose. To say: Expose allows... instead of Expose application allows...

Just like we say, e.g., Gitea provides... instead of Gitea application provides...

Commit: 46b4c53

@Mik-TF
Copy link
Contributor

Mik-TF commented Aug 7, 2024

@maayarosama Sorry for the direct commit. I understand that I should have made a review/proposition. Will use reviews in the future! Great work btw :)

Copy link
Contributor

@amiraabouhadid amiraabouhadid left a comment

Choose a reason for hiding this comment

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

please add a validation that ip should not be local host
image

@maayarosama maayarosama marked this pull request as draft August 8, 2024 13:26
Comment on lines 31 to 35
<SelectSolutionFlavor
v-model="solution"
:small="{ cpu: 1, memory: 2, disk: 50 }"
:medium="{ cpu: 2, memory: 4, disk: 100 }"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

this solution shouldn't contain a vm, it should be only a gateway workload

Comment on lines 85 to 92
tooltip="User's machine's public IP , It could be Mycelium IP, Yggdrasil IP, or a public IP (IPv4 or IPv6)."
>
<input-validator
:value="ip"
:rules="[validators.required('Public IP is required.'), validators.isIP('Public IP is not valid.')]"
#="{ props }"
>
<v-text-field label="Public IP" v-model="ip" v-bind="props" />
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't have to be a public ip. Mycelium and Yggdarsil are not public. let's rename it to IP


await deployGatewayName(grid, selectionDetails.value.domain, {
subdomain: subdomain.value,
ip: ip.value,
Copy link
Contributor

Choose a reason for hiding this comment

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

the ipv4 here will work as it will be in the format http://{ip}:{port}
but in case of the ipv6, it won't work as it should be http://[{ip}]:{port}

@maayarosama maayarosama marked this pull request as ready for review August 18, 2024 12:28
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

I can't delete a created domain instance,

Screencast.from.2024-08-19.10-58-34.webm

title: Domains
---

Domains allows users to securely expose servers hosted on local machines or VMs to the public internet. Users are required to specify the machine's IP, which can be a Mycelium IP, Yggdrasil IP, or a public IP (IPv4 or IPv6). For more details, check the [Domains documentation](https://www.manual.grid.tf/documentation/dashboard/solutions/expose.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the documentation file name in info_grid repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the changes in this file?

packages/playground/src/utils/delete_deployment.ts Outdated Show resolved Hide resolved
packages/playground/src/weblets/tf_domains.vue Outdated Show resolved Hide resolved
@AhmedHanafy725
Copy link
Contributor

please fix the failing build

@0oM4R
Copy link
Contributor

0oM4R commented Aug 20, 2024

image

still exists

@0oM4R
Copy link
Contributor

0oM4R commented Aug 20, 2024

image

I tried to use custom domain, but i found this after deployment succeed

Screenshot from 2024-08-19 11-15-31

Screenshot from 2024-08-19 11-15-43

deployment data: Screenshot from 2024-08-19 11-17-06

- chore: Update default value for 'documentScrollend' to 'false' as 'true' casuing it to not working fine in most cases
- fix: Domains solution now show data after deploying
@0oM4R
Copy link
Contributor

0oM4R commented Aug 20, 2024

I tried to use custom domain, but i found this after deployment succeed

Screenshot from 2024-08-19 11-15-31

Screenshot from 2024-08-19 11-15-43

deployment data: Screenshot from 2024-08-19 11-17-06

Screencast.from.2024-08-20.13-06-55.webm

Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

the behavior of deleting the deployments is not consist with other solutions.

Screencast.from.2024-08-20.13-12-44.webm

@Mik-TF
Copy link
Contributor

Mik-TF commented Aug 20, 2024

Some text suggestions:

image

  • Alternative: not points but point

image

  • This should be two sentences (missing a dot). Proposition: "Machine's IP: It could be a Mycelium IP, an Yggdrasil IP or a public IP (IPV4 or IPv6)"

General notes: some of those details button have the texts ending with a dot and others don't. We might want to generalize the presenting (ending or not with a dot for all).

image

image

  • Proposition for below: "The port used..."
    image

  • "Selecting Custom Domain sets the subdomain as the gateway name"

image

@0oM4R
Copy link
Contributor

0oM4R commented Aug 22, 2024

image

why do we need the sshkey field here is it used ?

@0oM4R
Copy link
Contributor

0oM4R commented Aug 25, 2024

please add tls passthrough option field

- chore: removed unused ssh
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

Thank you both for the great work

@@ -186,4 +191,8 @@ export const deploymentListEnvironments = {
SSH_KEY: _ssh,
NODE_PILOT_HOSTNAME: "Node Pilot Hostname",
},
domains: {
SSH_KEY: _ssh,
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this, i think we don't need it

Copy link
Contributor

@amiraabouhadid amiraabouhadid left a comment

Choose a reason for hiding this comment

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

@amiraabouhadid amiraabouhadid self-requested a review August 28, 2024 13:28
@amiraabouhadid
Copy link
Contributor

why can't i toggle the TLS switch ? https://www.loom.com/share/8f66f5c0898246dfab389c4b7a2687b4?sid=80b3e425-10a8-4aa6-8523-093e98f71d07

my bad didn't realize this switch is read only

@MohamedElmdary MohamedElmdary merged commit 448a213 into development Aug 28, 2024
10 checks passed
@MohamedElmdary MohamedElmdary deleted the development_expose_application branch August 28, 2024 13:37
@xmonader xmonader added this to the 2.6.0 milestone Sep 24, 2024
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.

7 participants