#631: remove -D on server add #448

2024-11-26T01:25:50Z

ammaratef45 commented

2024-11-26 01:25:50 +00:00

toolshed/organising#631

https://git.coopcloud.tech/toolshed/organising/issues/631

🎉 2

ammaratef45 added 1 commit 2024-11-26 01:25:51 +00:00

remove -D on server add

continuous-integration/drone/pr Build is failing

Details

8ee1947fe9

ammaratef45 referenced this pull request from toolshed/organising

2024-11-26 01:28:35 +00:00

Remove --no-domain-checks; warn on `abra server add` #631

decentral1se reviewed 2024-11-26 10:59:32 +00:00

decentral1se left a comment

Nice one @ammaratef45! That's a great start.

There are however still some places where domain checks are carried out. See EnsureDomainsResolveSameIPv4 also under abra app deploy ... which relates to toolshed/organising#631 (comment).

I think taking a scan of pkg/dns/..., seeing what is in there in terms of functions and where those get called will give you an overview of the logic. I think the idea is that domain checking happens once on the abra server add and from then on in, it's "up to you".

/cc @p4u1 @fauno

Nice one @ammaratef45! That's a great start. There are however still some places where domain checks are carried out. See `EnsureDomainsResolveSameIPv4` also under `abra app deploy ...` which relates to https://git.coopcloud.tech/toolshed/organising/issues/631#issuecomment-21662. I think taking a scan of `pkg/dns/...`, seeing what is in there in terms of functions and where those get called will give you an overview of the logic. I think the idea is that domain checking happens once on the `abra server add` and from then on in, it's "up to you". /cc @p4u1 @fauno

cli/server/add.go Outdated

						
				@@ -122,3 +119,1 @@

				of your ~/.ssh/config. Checks for a valid online domain will be skipped:

				  abra server add -D example`,

				`,

decentral1se commented

2024-11-26 10:56:05 +00:00

Might seem like a nitpick but this "`" needs to be on the same line as the "default" or else a newline is created in the help output of abra which is inconsistent with the rest of the commands as far as I remember 🙃

Might seem like a nitpick but this "\`" needs to be on the same line as the `"default"` or else a newline is created in the help output of `abra` which is inconsistent with the rest of the commands as far as I remember 🙃

ammaratef45 commented

2024-11-27 19:39:18 +00:00

Ah, mia culpa

ammaratef45 marked this conversation as resolved

ammaratef45 commented

2024-11-26 18:17:50 +00:00

I think taking a scan of pkg/dns/..., seeing what is in there in terms of functions and where those get called will give you an overview of the logic. I think the idea is that domain checking happens once on the abra server add and from then on in, it's "up to you".

I thought the idea is that domain checking results only in a warning for server add but since apps can have domains that are not subdomains of the server then we need to keep domain checking for them?

Or do we want to have this as a warning in all of the cases? I could just remove the -D parameter internal/cli and go chase everywhere where an error shows up if that's the goal

> I think taking a scan of pkg/dns/..., seeing what is in there in terms of functions and where those get called will give you an overview of the logic. I think the idea is that domain checking happens once on the abra server add and from then on in, it's "up to you". I thought the idea is that domain checking results only in a warning for `server add` but since apps can have domains that are not subdomains of the server then we need to keep domain checking for them? Or do we want to have this as a warning in all of the cases? I could just remove the `-D` parameter `internal/cli` and go chase everywhere where an error shows up if that's the goal

p4u1 approved these changes 2024-11-26 19:48:34 +00:00

decentral1se commented

2024-11-27 14:55:50 +00:00

@ammaratef45 I understood from toolshed/organising#631 (comment) that perhaps the additional domain checks on deploy were also a hindrance? @fauno will hopefully weigh in on this shortly.

We can also just merge as-is as @p4u1 is suggesting (this change stands on its own and is good anyway pending coop-cloud/abra#448 (comment))! Feel free to click the big green button!

@ammaratef45 I understood from https://git.coopcloud.tech/toolshed/organising/issues/631#issuecomment-21662 that perhaps the additional domain checks on deploy were also a hindrance? @fauno will hopefully weigh in on this shortly. We can also just merge as-is as @p4u1 is suggesting (this change stands on its own and is good anyway pending https://git.coopcloud.tech/coop-cloud/abra/pulls/448#issuecomment-21729)! Feel free to click the big green button!

ammaratef45 added 1 commit 2024-11-27 19:38:56 +00:00

remove whitespace

continuous-integration/drone/pr Build is failing

Details

7b54c2b5b9

ammaratef45 commented

2024-11-27 19:41:14 +00:00