fix(valheim): fix open portal button

[opello]

Description
The "Open" link previously opened the STATUS_HTTP_PORT, which serves a
status.json generated by querying the Valheim server's query port. Now
it opens the SUPERVISOR_HTTP_PORT which presents a status and control
web interface for the various processes running under the supervisor.

This seems much more useful than the blank default and JSON file
available in the other server.

⚙️ Type of change

• ⚙️ Feature/App addition
• 🪛 Bugfix
• ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 🔃 Refactor of current code

🧪 How Has This Been Tested?

.github/scripts/build-catalog.sh stable/valheim
scp catalog/stable/valheim/5.0.4/questions.yaml truenas.local:/mnt/data/ix-applications/catalogs/github_com_truecharts_catalog_main/stable/valheim/5.0.3/questions.yaml

Then added a new "valheim-test" app based on that locally modified questions.yaml. The open button behaved as I'd like instead of opening a 404 page.

📃 Notes:
There are additional details here:
https://github.com/lloesche/valheim-server-docker#status-web-server

✔️ Checklist:

• ⚖️ My code follows the style guidelines of this project
• 👀 I have performed a self-review of my own code
• #️⃣ I have commented my code, particularly in hard-to-understand areas
• 📄 I have made corresponding changes to the documentation
• ⚠️ My changes generate no new warnings
• 🧪 I have added tests to this description that prove my fix is effective or that my feature works
• ⬆️ I increased versions for any altered app according to semantic versioning

[CLAassistant]

All committers have signed the CLA.

[xstar97]

removed the option from the question to disable the supervisor service since its now the main service and status is secondary.

[opello]

removed the option from the question to disable the supervisor service since its now the main service and status is secondary.

I wasn't sure if this was the right path when it would affect the "health checks" or however that's all plugged together.

[xstar97]

removed the option from the question to disable the supervisor service since its now the main service and status is secondary.

I wasn't sure if this was the right path when it would affect the "health checks" or however that's all plugged together.

lets discuss on discord -> https://discord.gg/JfjxEcFb

[opello]

The only other thing that it seems might be worth adding in this PR is preventing someone from disabling the supervisord HTTP port. That's a bit of a built-in foot-gun with it still allowed.

[xstar97]

The only other thing that it seems might be worth adding in this PR is preventing someone from disabling the supervisord HTTP port. That's a bit of a built-in foot-gun with it still allowed.

That's been removed in the gui from my prs We need to reference it still in the values.yaml

You need to pull first to get changes, it seems like you're resetting every commit i pushed.

[stavros-k]

I'm a bit hesitant on this in general.

Breaking changes to just alter a button... I'm not a fan

[xstar97]

i removed the variable thats set the backup dir in the GUI since we mount the predefined location in values as /backups

[stavros-k]

Suggested change

	default: ''
	default: ""

[opello]

Should these defaults still be removed under the scope of this PR?

[opello]

You need to pull first to get changes, it seems like you're resetting every commit i pushed.

I'm well aware, I specifically rewrote and reorganized the work. I thought that would be obvious from the history I pushed.

[opello]

Breaking changes to just alter a button... I'm not a fan

I'm not sure what this means. What exactly is the breaking change? I kind of scope-crept the work here to "fix up" the chart. It seemed worth improving a bit if I was spending some time on it.

[stavros-k]

Breaking changes to just alter a button... I'm not a fan

I'm not sure what this means. What exactly is the breaking change? I kind of scope-crept the work here to "fix up" the chart. It seemed worth improving a bit if I was spending some time on it.

Well changing service names etc, might be ok in the kubernetes side, but there is a big chance SCALE UI won't be that happy when someone upgrades the app.

Also, people with enabled ingress on the main service, which was the status service, suddenly they will have ingress in the supervisor instead.

[opello]

Breaking changes to just alter a button... I'm not a fan

I'm not sure what this means. What exactly is the breaking change? I kind of scope-crept the work here to "fix up" the chart. It seemed worth improving a bit if I was spending some time on it.

Well changing service names etc, might be ok in the kubernetes side, but there is a big chance SCALE UI won't be that happy when someone upgrades the app.

Is there a practical concern or a way to measure whether or not the upgrade path is adversely affected?

Also, people with enabled ingress on the main service, which was the status service, suddenly they will have ingress in the supervisor instead.

This makes sense. It seemed like both would have to change in parallel. Is that true? I asked @xstar97 if the "main" name was special. If it wasn't, or didn't have to be, the names could be "status" and "supervisor" instead of "main" and "the other thing." That way the intent would be independent of the order of the things in the YAML.

[stavros-k]

Is there a practical concern or a way to measure whether or not the upgrade path is adversely affected?

Experience only, GUI is not something we built, so we don't really know how it will react.
It surely not when a type of a variable changes, eg int to string, string to bool, etc even if the value is "true" for example.
Even if you add an enum in a string, it's not very happy.

In this case there is a chance it won't complain, as it's essentially removing one variable and adding another.
But I can't guarantee

This makes sense. It seemed like both would have to change in parallel. Is that true? I asked @xstar97 if the "main" name was special. If it wasn't, or didn't have to be, the names could be "status" and "supervisor" instead of "main" and "the other thing." That way the intent would be independent of the order of the things in the YAML.

main is special in our common lib. on all objects.

[opello]

This makes sense. It seemed like both would have to change in parallel. Is that true? I asked @xstar97 if the "main" name was special. If it wasn't, or didn't have to be, the names could be "status" and "supervisor" instead of "main" and "the other thing." That way the intent would be independent of the order of the things in the YAML.

main is special in our common lib. on all objects.

Okay. Such a convention must provide immense value to be preferred to strong semantic meaning carried by well named items? Because it seems to open the door for this kind of situation as things get more complex... Is there a preferable or more idiomatic way forward? Or "change isn't allowed" is the takeaway?

You can see my first approach:
668d973
2c1088d
1cc67a1

It was a much more minimal change.

[stavros-k]

This makes sense. It seemed like both would have to change in parallel. Is that true? I asked @xstar97 if the "main" name was special. If it wasn't, or didn't have to be, the names could be "status" and "supervisor" instead of "main" and "the other thing." That way the intent would be independent of the order of the things in the YAML.

main is special in our common lib. on all objects.

Okay. Such a convention must provide immense value to be preferred to strong semantic meaning carried by well named items? Because it seems to open the door for this kind of situation as things get more complex... Is there a preferable or more idiomatic way forward? Or "change isn't allowed" is the takeaway?

You can see my first approach: 668d973 2c1088d 1cc67a1

It was a much more minimal change.

While the variable approach is not the best, because it won't work when ClusterIP + ingress is setup.
It's ok for now.

Portal configmap will be (re-)worked in the near future, to correctly generate for all services.

So, yea, I'd prefer minimal changes, instead of having floods of tickets for errors on upgrade.

[opello]

While the variable approach is not the best, because it won't work when ClusterIP + ingress is setup. It's ok for now.

So, yea, I'd prefer minimal changes, instead of having floods of tickets for errors on upgrade.

So, which is the future for this PR? The minimal change but not getting to use the portalLink template or as-is with the reordering and additional clean up and rework?

Or adding an additional button besides "Open" like "Admin" in some others? If so, what would be the preferred label?

Portal configmap will be (re-)worked in the near future, to correctly generate for all services.

In this near future world of a re-worked Portal configmap, what does selecting the visible actions on the applications page look like? Does each service get an entry in a list? Are they captioned for the service label? Seems like it would be space constrained...

[stavros-k]

In this near future world of a re-worked Portal configmap, what does selecting the visible actions on the applications page look like? Does each service get an entry in a list? Are they captioned for the service label? Seems like it would be space constrained...

It will be just the configmap generated. What buttons will show will be up to the dev to decide.

For now, single button, pointed to the supervisor port. Is there a valid reason to have button for the status page?

[opello]

For now, single button, pointed to the supervisor port. Is there a valid reason to have button for the status page?

Not that I can see. It certainly is not useful as-is in master.

And how do you want that achieved? I'm just looking for guidance on the approach that will be palatable at this point, before updating the PR.

[stavros-k]

For now, single button, pointed to the supervisor port. Is there a valid reason to have button for the status page?

Not that I can see. It certainly is not useful as-is in master.

And how do you want that achieved? I'm just looking for guidance on the approach that will be palatable at this point, before updating the PR.

2c1088d

Like this.. It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

[opello]

It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

And what exactly is the fix for that? Is the problem that the name association of "main" "supervisor" and "status" is preserved when ingress is configured? Or is there some way to avoid the surprise behavior or inconsistency? Or given that should the major number change (and then maybe we justify the bigger changes) ...?

[stavros-k]

It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

And what exactly is the fix for that?

it's the portal configmap generation. common library side.
I'm refactoring the library already.

Is the problem that the name association of "main" "supervisor" and "status" is preserved when ingress is configured? Or is > there some way to avoid the surprise behavior or inconsistency? Or given that should the major number change (and then > maybe we justify the bigger changes) ...?

I don't really see a good reason to have "breaking" changes. Even if you change the names on the services and it works now. That just hides the inability for the portal generator to create configmaps for other services.

Anyway.

[opello]

Is the problem that the name association of "main" "supervisor" and "status" is preserved when ingress is configured? Or is > there some way to avoid the surprise behavior or inconsistency? Or given that should the major number change (and then > maybe we justify the bigger changes) ...?

I don't really see a good reason to have "breaking" changes. Even if you change the names on the services and it works now. That just hides the inability for the portal generator to create configmaps for other services.

Anyway.

But the use of a "main" is inherently misleading and obfuscating to the intent of other parts of the chart definition. It makes reasoning about it harder than necessary due to the sentinel name. That's why I was talking about the value provided by the shared infrastructure must be worth more than the cognitive load required to understand the system. This is a judgement call for you guys, but as someone that is just approaching it and not finding it documented, it was difficult to grok.

Furthermore, you didn't really describe the issue with ingress. Is it because of the name "main" or because of the order of things in another list or something altogether different? I asked because I don't know, and it's just as time expensive to guess when you could describe the problem. I did ask.

Breaking things is really easily justified when the thing that is broken was not well defined. I would argue that the "Open" button not behaving like other "Open" buttons (presenting some sort of control surface of the app) is very broken. So much so that I'm trying to fix it, and to do so in whatever way makes the most sense to be consistent with the other aspects of this environment, aspects of which I am nowhere near as familiar as you or others.

[opello]

Like this.. It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

I think in the current form of this PR ingress should not be adversely affected. Please let me know if that's wrong.

[stavros-k]

Like this.. It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

I think in the current form of this PR ingress should not be adversely affected. Please let me know if that's wrong.

Ingress is on port 443, the variable won't point to that port.
So when ingress enabled, it will point to DNS:supervisor_port instead of DNS:443
But, nothing to do now. I'll handle it on the common lib side

[opello]

Like this.. It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

I think in the current form of this PR ingress should not be adversely affected. Please let me know if that's wrong.

Ingress is on port 443, the variable won't point to that port.
So when ingress enabled, it will point to DNS:supervisor_port instead of DNS:443
But, nothing to do now. I'll handle it on the common lib side

Okay, but just to be perfectly clear that isn't a change born out of this PR. It was like that before and like that it remains. It was unclear as to if you were attributing the fact to this PR or just a general comment about the state of this chart.

Nothing in the container binds to 443 so that doesn't seem like an applicable fix for this chart anyway.

[stavros-k]

Like this.. It will still not work when ingress is enabled on supervisor service, but that will be fixed later.

I think in the current form of this PR ingress should not be adversely affected. Please let me know if that's wrong.

Ingress is on port 443, the variable won't point to that port.
So when ingress enabled, it will point to DNS:supervisor_port instead of DNS:443
But, nothing to do now. I'll handle it on the common lib side

Okay, but just to be perfectly clear that isn't a change born out of this PR. It was like that before and like that it remains. It was unclear as to if you were attributing the fact to this PR or just a general comment about the state of this chart.

Nothing in the container binds to 443 so that doesn't seem like an applicable fix for this chart anyway.

Ingress (aka Reverse proxy) listens on 443 (or whatever the port is set in traefik).
So yea, when ingress is enabled on supervisor service, it will try to open on https://ingresshost:443

[truecharts-admin]

This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this