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

Enhance networks Component #3344

Draft
wants to merge 20 commits into
base: development
Choose a base branch
from

Conversation

MohamedElmdary
Copy link
Member

@MohamedElmdary MohamedElmdary commented Aug 28, 2024

Description

Enhance networks Component

Changes

  • Make component more simpler
  • Add an easier way to tell if it's readonly or not
// by default all networks `false` but mycelium
// pass object to update any default like below
const { ipv4, ipv6, wireguard, planetary, mycelium } = useNetworks({ ipv4: true });
<Networks v-model:mycelium="mycelium" v-model:planetary="planetary" :ipv4="ipv4" v-model:ipv6="ipv6" />

image

Related Issues

Documentation PR

For UI changes, Please provide the Documetation PR on info_grid

Checklist

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

Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad left a comment

Choose a reason for hiding this comment

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

Good job ya MR-Rabeeee3

packages/playground/src/components/networks.vue Outdated Show resolved Hide resolved
@Mahmoud-Emad
Copy link
Contributor

@ALL
Please let's merge #3287 first

packages/playground/src/weblets/micro_vm.vue Outdated Show resolved Hide resolved
packages/playground/src/components/networks.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad left a comment

Choose a reason for hiding this comment

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

Teslam edak ya Rabe3

const ipv6 = ref(false);
const mycelium = ref(true);
const planetary = ref(true);
const { ipv4, ipv6, mycelium, planetary } = useNetworks({ planetary: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

require-domain should be added to the Networks tag in template.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. require-domain isn't a network so it shouldn't be used in useNetworks
  2. require-domain won't change for specific solution (solution will need domain or not)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed but it is only useful for the tooltip it shows here:

const wireguardTooltip = computed(() =>
props.requireDomain
? "Enabling WireGuard Access allows you to establish private, secure, and encrypted instance connections. Please note that this field will be read-only unless you use a custom domain with IPV4."
: "Enabling WireGuard Access allows you to establish private, secure, and encrypted instance connections.",
);

Copy link
Member Author

Choose a reason for hiding this comment

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

it's also used as readonly for wireguard

@0oM4R
Copy link
Contributor

0oM4R commented Sep 2, 2024

Screencast.from.2024-09-02.14-26-43.webm

the only case we should have the wg not readonly is: when having ipv4 with customDomain enabled

packages/playground/src/weblets/freeflow.vue Outdated Show resolved Hide resolved
);
expose({
error,
const error = computed(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

on k8s I was able to disable all networks and click on deploy without any errors
image

Copy link
Member Author

Choose a reason for hiding this comment

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

this is because it's not required but I checked early versions and it wasn't require too should I turn it into required?

tooltip-text="An Internet Protocol version 4 address that is globally unique and accessible over the internet."
label="Public IPv4"
:value="$props.ipv4"
:emit-function="$attrs['onUpdate:ipv4']"
Copy link
Contributor

Choose a reason for hiding this comment

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

on caprover we need the ipv4 to be read only

Copy link
Member Author

Choose a reason for hiding this comment

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

should be fixed

@0oM4R
Copy link
Contributor

0oM4R commented Sep 11, 2024

In k8s worker and leader i still can disable all network options
as we decided in the BAM we have to change this behavior, not sure it was to force wg or mycelium
Screenshot from 2024-09-11 15-09-21

@0oM4R
Copy link
Contributor

0oM4R commented Sep 11, 2024

also I was able to deploy an algorand instance while disable all network options,

@0oM4R
Copy link
Contributor

0oM4R commented Sep 11, 2024

image
is this normal we don't have wg at all in Jenkins

@zaelgohary zaelgohary marked this pull request as draft September 19, 2024 05:52
add wireguard,
set wg value to false when readonlywireguard is false
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.

4 participants