Skip to content

feat(filebrowser): support subdomain = false #286

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

Merged
merged 21 commits into from
Sep 19, 2024
Merged

feat(filebrowser): support subdomain = false #286

merged 21 commits into from
Sep 19, 2024

Conversation

Seppdo
Copy link
Contributor

@Seppdo Seppdo commented Aug 29, 2024

Resolves #285.

Add variables workspace_name, owner_name and resource_name to be able to infer the directory the module is served at. Set this directory in the filebrowser app via the filebrowser config set --baseurl function.

Added an example to readme.

@Seppdo Seppdo changed the title filebrowser: allow to use subdomain = false feat(filebrowser): support subdomain = false Aug 30, 2024
Copy link
Member

@matifali matifali 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 @Seppdo for the contribution.
I have requested a few changes to make it more optimized.

Comment on lines 48 to 59
### Serve from the same domain (no subdomain)

Make sure to set both workspace_name and owner_name.

```tf
module "filebrowser" {
source = "registry.coder.com/modules/filebrowser/coder"
agent_id = coder_agent.main.id
workspace_name = data.coder_workspace.me.name
owner_name = data.coder_workspace_owner.me.name
}
```
Copy link
Member

@matifali matifali Aug 30, 2024

Choose a reason for hiding this comment

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

Also update the example accordingly.

Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

You may have to adjust a few places.

@matifali
Copy link
Member

You can also check my review for #288 and do a similar approach. In this case you can discard the requested changes.

@matifali matifali self-requested a review September 16, 2024 18:06
Seppdo and others added 2 commits September 17, 2024 09:45
Add missing subdomain variable
- Fix subdomain variable expected as number by count in data fields
- Add test case for subdomain=false
@Seppdo Seppdo marked this pull request as draft September 17, 2024 08:32
Seppdo and others added 2 commits September 18, 2024 09:20
Co-authored-by: Muhammad Atif Ali <[email protected]>
- Add agent_name to test cases since it is a required variable (even on those cases where it is not actually needed)
- Prettify README.md
@Seppdo
Copy link
Contributor Author

Seppdo commented Sep 18, 2024

Since agent_name is a required parameter, it must be set even in cases where it is not actually needed (e.g. every case with subdomain=true)

I think users are fine with this, especially since they can just copy the example in the readme.

@Seppdo Seppdo marked this pull request as ready for review September 18, 2024 08:15
@Seppdo
Copy link
Contributor Author

Seppdo commented Sep 18, 2024

I don't know how to fix the error in the pretty check
error: script "fmt:ci" exited with code 3

@matifali
Copy link
Member

@Seppdo can you run bun i && bun fmt?

@matifali matifali merged commit 834ffde into coder:main Sep 19, 2024
2 checks passed
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.

Filebrowser module: support for subdomain = false
2 participants