mehrancodes / laravel-harbor

a cli tool to Quickly create on-demand preview environment for your app on Laravel Forge.
https://www.laravel-harbor.com
MIT License
75 stars 7 forks source link

Branch name syntax #25

Closed mgkimsal closed 11 months ago

mgkimsal commented 12 months ago

Related to #24 , possibly, branch names (the github_ref, IIRC) can't seem to have / in them.

Making a PR against develop with branch name "feature/374-update-user-count" - for example - doesn't seem to work.

mehrancodes commented 12 months ago

Merged changes: 🔥 View Pull Request.

Additionally, if you wish to omit the feature/ section and the -update-user-count, consider setting the FORGE_SUBDOMAIN_PATTERN to "/\d+/". This ensures a cleaner subdomain.

mgkimsal commented 12 months ago

i saw the 'subdomain' pattern ability, but it's not understandable to me. more doc examples to deal with these common patterns might help, but overall, the Str::slug() seemed like it was intended, but it was an oversight. That was my understanding anyway.

mehrancodes commented 12 months ago

After provisioning a branch like "feature/374-update-user-count", you might get a lengthy subdomain like www.feature374-update-user-count.veyoze.com.

Want a simpler domain? Set the FORGE_SUBDOMAIN_PATTERN to a regex like '/(feature/\d+)/i', and you'll get something like feature325.veyoze.com.

I'm thinking of replacing slashes with dashes for domain and database names to keep things tidy. e.g., Domain would be feature-325.veyoze.com. I'll work on this enhancement tomorrow and update you soon!

Also a reminder I put here to update the documentation as well 🛠️

mgkimsal commented 12 months ago

Unsure you should visually add the "www" prefix.

It never works for me. I have wildcard for *.preview.domain.com

When deploying feature-test-deply.preview.domain.com, the letsencrypt process makes a cert for feature-test-deply.preview.domain.com, but not for www.feature-test-deply.preview.domain.com. Trying to access the URL that is displayed at the end doesn't work, because it prepends "www." at the front.

mehrancodes commented 12 months ago

The www prefix gets added during provisioning, but it's not needed. I'll remove it. I've had some challenges with Let'sEncrypt for nested subdomains. I'll revisit this to recall my solution.

Any further suggestions for the domain?

mgkimsal commented 12 months ago

I mostly work with 'feature/' prefixed branches (as do some other folks I work with) and would just like to get rid of that leading portion, but keep everything else ('update-user-count','fix-ui-timeout', etc). unsure how to do that (doc update with example appreciated).

mehrancodes commented 12 months ago

It is possible to do so. The regex /feature\/\d+-(.+)/ will extract "update-user-count" from "feature/374-update-user-count".

mehrancodes commented 12 months ago

Also having FORGE_SUBDOMAIN_PATTERN=/feature\/(.+)/, leads to remove only the leading portion, and results in "374-update-user-count".

I hope that has been helpful. Would be great to kinda address all of these types in the DOC I think 🧐🛠️

mgkimsal commented 11 months ago

patch at https://github.com/mehrancodes/veyoze/pull/36

I couldn't contribute a test for this yet.

Some notes from that PR

Consider

$branch = "feature/foo-bar-abz";
$pattern = "/feature/(.*)/";

if (isset($pattern)) {
  preg_match($pattern, $branch, $matches);
}

$matches contains

Array
(
  [0] => feature/foo-bar-abz
  [1] => foo-bar-abz
)

The last match seems to be generally what we’d want.

New method is

private function formatBranchName(): string
    {
        $branch = $this->service->setting->branch;
        $pattern = $this->service->setting->subdomainPattern;

        if (isset($pattern)) {
            preg_match($pattern, $branch, $matches);
            $branch = array_pop($matches);
        }

        return Str::slug($branch);
    }

single return point.

mehrancodes commented 11 months ago

Thanks for https://github.com/mehrancodes/veyoze/pull/36. Marking this issue as fixed 🔥🛠️