3wordchant/capsul-flask
1
0
Fork 1
Code Issues Pull Requests 1 Releases Wiki Activity

Make the displayed SSH username configurable #12

Merged
mirsal merged 1 commits from mirsal/capsul-flask:ssh-username into master 2021-08-10 09:56:15 +00:00
Conversation 2 Commits 1 Files Changed 5 +8 -5
mirsal commented 2021-08-09 21:36:04 +00:00
Collaborator
Copy Link

This patch allows the SSH username displayed in templates to be
configured through the SSH_USERNAME environment variable.

This patch allows the SSH username displayed in templates to be configured through the SSH_USERNAME environment variable.
mirsal requested review from j3s 2021-08-09 21:37:43 +00:00
mirsal requested review from forest 2021-08-09 21:37:43 +00:00
3wordchant approved these changes 2021-08-09 21:53:36 +00:00
3wordchant left a comment
Owner
Copy Link

LGTM! Maybe one stylistic change, we could potentially fix that after. @forest over to you 👌

LGTM! Maybe one stylistic change, we could potentially fix that after. @forest over to you 👌
capsulflask/templates/capsul-detail.html Outdated
@ -98,3 +98,3 @@
<div class="row justify-start">
<label class="align" for="ssh_username">SSH Username</label>
<span id="ssh_username">cyberian</span>
<span id="ssh_username">{{ config.SSH_USERNAME | default('cyberian') }}</span>
3wordchant commented 2021-08-09 21:52:23 +00:00
Owner
Copy Link

I think @forest was saying he preferred that templates not be dependent on the structure of the config, so setting a context variable in the view as here: https://git.autonomic.zone/3wordchant/capsul-flask/src/branch/master/capsulflask/console.py#L429

I think @forest was saying he preferred that templates not be dependent on the structure of the config, so setting a context variable in the view as here: https://git.autonomic.zone/3wordchant/capsul-flask/src/branch/master/capsulflask/console.py#L429
mirsal commented 2021-08-09 23:37:24 +00:00
Author
Collaborator
Copy Link

it can be done that way, although there are many calls to render_template() and so adding a context variable to each of them would be error-prone and require many more changes.

@forest would you rather have it passed through the context every time?

it can be done that way, although there are many calls to render_template() and so adding a context variable to each of them would be error-prone and require many more changes. @forest would you rather have it passed through the context every time?
forest commented 2021-08-09 23:59:51 +00:00
Collaborator
Copy Link

hmm.. I don't think it's any more error prone than putting the config.SSH_USERNAME into the template. I have been hurt by template engines too many times and I tend to try to isolate and validate all inputs into the template before I execute it. In the past, I've encountered template engines that fail silently, or fail with a meaningless error (there is no stack trace inside the template, inside the template no one can hear you scream).

TBH I did not know that it was even possible to reach into a global config object from within the template. In my mental model, the only thing you can access from within the template is exactly what is directly passed to it. Obviously thats not true in flask, but if I could make it true, I would. When dealing with error-prone things that do not always provide good error messages, having a comprehensive inventory of all the moving parts in one place helps a lot.

I think it would help developers debug templates easier if they could simply inspect the data object that gets passed to the template right before it is executed, either through print debugging (logger.debug(object_to_json_string(data_object))) or with a property inspector in a proper debugger.

I guess I believe that its the same error-prone, duplicate code problem either way, but I am strongly biased against template engines and I want to put them all in diapers, even though I don't have enough experience with jinja2 to know for sure if it needs that kind of treatment.

Is this silly? Am I wrong? I don't know. I have been doing it one way and I'm resistant to change 😳

hmm.. I don't think it's any more error prone than putting the ` config.SSH_USERNAME ` into the template. I have been hurt by template engines too many times and I tend to try to isolate and validate all inputs into the template before I execute it. In the past, I've encountered template engines that fail silently, or fail with a meaningless error (there is no stack trace inside the template, inside the template no one can hear you scream). TBH I did not know that it was even possible to reach into a global `config` object from within the template. In my mental model, the only thing you can access from within the template is exactly what is directly passed to it. Obviously thats not true in flask, but if I could make it true, I would. When dealing with error-prone things that do not always provide good error messages, having a comprehensive inventory of all the moving parts in one place helps a lot. I think it would help developers debug templates easier if they could simply inspect the data object that gets passed to the template right before it is executed, either through print debugging (`logger.debug(object_to_json_string(data_object))`) or with a property inspector in a proper debugger. I guess I believe that its the same error-prone, duplicate code problem either way, but I am strongly biased against template engines and I want to put them all in diapers, even though I don't have enough experience with jinja2 to know for sure if it needs that kind of treatment. Is this silly? Am I wrong? I don't know. I have been doing it one way and I'm resistant to change 😳
mirsal commented 2021-08-10 00:35:11 +00:00
Author
Collaborator
Copy Link

That's definitely not silly and I do not believe that you are wrong (I think that it is safe in that specific case but it would not be obvious to future contributors)

I'm sending another patch which uses the vm context object instead of adding a new context variable, so we get the best of both worlds ;)

That's definitely not silly and I do not believe that you are wrong (I think that it is safe in that specific case but it would not be obvious to future contributors) I'm sending another patch which uses the `vm` context object instead of adding a new context variable, so we get the best of both worlds ;)
forest marked this conversation as resolved
capsulflask/templates/faq.html Outdated
@ -23,3 +23,2 @@
How do I log in?
<p>ssh to the ip provided to you using the cyberian user.</p>
<pre class='code'>$ ssh cyberian@1.2.3.4</pre>
<p>ssh to the ip provided to you using the "{{ config.SSH_USERNAME | default('cyberian') }}" user.</p>
3wordchant commented 2021-08-09 21:52:35 +00:00
Owner
Copy Link

As above

As above
mirsal marked this conversation as resolved
mirsal force-pushed ssh-username from 837660aad9 to d4a9f2f40a 2021-08-10 00:31:23 +00:00 Compare
forest commented 2021-08-10 00:43:40 +00:00
Collaborator
Copy Link

👍

👍
❤️ 1
j3s commented 2021-08-10 00:51:53 +00:00
Collaborator
Copy Link

Love this. Maybe there should be a linked set of changes to modify the capsul-images repo in order to produce images with the given username as well. Otherwise this all LGTM!

Love this. Maybe there should be a linked set of changes to modify the capsul-images repo in order to produce images with the given username as well. Otherwise this all LGTM!
j3s approved these changes 2021-08-10 00:52:19 +00:00
forest approved these changes 2021-08-10 01:01:58 +00:00
mirsal merged commit 8a944104d3 into master 2021-08-10 09:56:15 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
3wordchant
j3s
forest
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: 3wordchant/capsul-flask#12
No description provided.
Delete Branch "mirsal/capsul-flask:ssh-username"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?