Skip to content

Conversation

bbrala
Copy link
Contributor

@bbrala bbrala commented Sep 21, 2022

The Problem/Issue/Bug:

I want to set a global project tld by default to support a custom TLD for out company.

How this PR Solves The Problem:

Adds a config global config option for project-tld that overwrites the default for the project and makes it globally settable.

Manual Testing Instructions:

Build, use --project-tld globally and see it work. You can also set it in the global config yaml.

Automated Testing Overview:

Added the option in the tests, but can't run tests locally, so thats a little bit hard.

I did test this manually by setting the global config in yml and observing our actual project tld be used! Hopefully i can use gitpod to run the tests.

Related Issue Link(s):

#2746

Release/Deployment notes:

@bbrala
Copy link
Contributor Author

bbrala commented Sep 21, 2022

Hmm, the tests were green, then pushed the whitespace fix, seems a unrelated issue?

@bbrala
Copy link
Contributor Author

bbrala commented Sep 21, 2022

@rfay
Copy link
Member

rfay commented Sep 21, 2022

The github tests don't even run because you're first-time contributor, so ping me when I need to run them again. Just started them. Congrats on the PR! After you get this PR in then you don't have to have tests run manually any more.

@bbrala
Copy link
Contributor Author

bbrala commented Sep 22, 2022

Yeah seems i was mistaken, there was actually a declaration that was removed for some reason. Guessing my infamiliarity with go.

@rfay
Copy link
Member

rfay commented Sep 22, 2022

Started tests.

@github-actions
Copy link

github-actions bot commented Sep 22, 2022

@bbrala bbrala marked this pull request as ready for review September 22, 2022 10:39
@bbrala
Copy link
Contributor Author

bbrala commented Sep 22, 2022

the fail seems unrelated to the code change:

get_test.go:47: 
        	Error Trace:	get_test.go:47
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestCmdGet
        	Messages:   	failed ddev get --list: exit status 1 (Failed to list available add-ons: Unable to get list of available services: GET https://api.github.com/search/repositories?q=topic:ddev-get+fork:true+org:drud: 403 You have exceeded a secondary rate limit. Please wait a few minutes before you try again. [] rateinfo=github.Rate{Limit:30, Remaining:28, Reset:github.Timestamp{2022-09-22 13:56:34 +0000 UTC}}
        	            	)
    get_test.go:48: 
        	Error Trace:	get_test.go:48
        	Error:      	"\x1b[31mFailed to list available add-ons: Unable to get list of available services: GET https://api.github.com/search/repositories?q=topic:ddev-get+fork:true+org:drud: 403 You have exceeded a secondary rate limit. Please wait a few minutes before you try again. [] rateinfo=github.Rate{Limit:30, Remaining:28, Reset:github.Timestamp{2022-09-22 13:56:34 +0000 UTC}}\x1b[0m\n" does not contain "drud/ddev-memcached"
        	Test:       	TestCmdGet

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Perfect, tested manually and it did great.

It still needs docs updated. I see references in https://ddev.readthedocs.io/en/latest/users/details/offline-usage/ and https://ddev.readthedocs.io/en/latest/users/configuration/config_yaml/ - but. it needs to be added to the global config on that page too.

Also needs to be added to global_config.yaml stuff in https://github.com/drud/ddev/blob/1cb188308d85f359285c917b0d24256cac55abbc/pkg/globalconfig/global_config.go#L210-L295

Awesome, thanks!

@rfay
Copy link
Member

rfay commented Oct 4, 2022

I'd like this to make it into the next point release (within the next week). Hope you can address the minor changes required.

@bbrala
Copy link
Contributor Author

bbrala commented Oct 4, 2022

Yeah i should be able to this week.

@bbrala
Copy link
Contributor Author

bbrala commented Oct 4, 2022

Hmm, I was also thinking though. I think right now global options take precedent. But perhaps this should not be the case. You might want a global option, but if you set something on the project level you probably want that value.

For this particular option it might now matter much. But especially if we look forwards, what about the php version. The global should perhaps just function as a configurable default value.

@rfay
Copy link
Member

rfay commented Oct 4, 2022

IMO global config should be a default, but there are cases where it isn't implemented that way, like mutagen. But you're certainly right that that's the obvious - global config should be default, and project config should be able to override it.

@bbrala
Copy link
Contributor Author

bbrala commented Oct 5, 2022

I've fixed all the requested changed afaik :) Ready for review again.

@rfay
Copy link
Member

rfay commented Oct 5, 2022

I rebased using the github UI, so you'll need to update your branch if further work is required. Github broke everybody by removing brew from the path lately, and your code didn't have that fix.

I see the docs and link checks failing though, you'll want to look at those.

@rfay
Copy link
Member

rfay commented Oct 5, 2022

I'll see if I can fix this by rolling back to a14223d and doing a rebase. Seems lots of things were lost.

@rfay rfay force-pushed the feature/global-project-tld branch from 2fd0665 to 419feb4 Compare October 5, 2022 16:24
@bbrala
Copy link
Contributor Author

bbrala commented Oct 5, 2022

Darnit. Ide was being silly then. If this doesn't work I can revert to master and cherry picker my changes if you like. Then all should be fine again.

Sometimes jetbrains works in mysterieus ways

@rfay
Copy link
Member

rfay commented Oct 5, 2022

I haven't sorted out what happened, but we'll get it.

@rfay
Copy link
Member

rfay commented Oct 5, 2022

Seems github was just slow to pick up my push, now it looks like all is OK. Except docs change landed that has conflicts. I'll fix that.

@bbrala
Copy link
Contributor Author

bbrala commented Oct 5, 2022

Thank you. :)

@rfay rfay force-pushed the feature/global-project-tld branch from 419feb4 to 7f97176 Compare October 5, 2022 17:54
@rfay
Copy link
Member

rfay commented Oct 5, 2022

Rebased, @mattstein feel free to take a look.

@rfay rfay requested a review from mattstein October 5, 2022 17:54
@mattstein
Copy link
Collaborator

Added two minor suggestions, otherwise looks great to me. Thanks @bbrala!

@rfay rfay force-pushed the feature/global-project-tld branch from 560724d to c423731 Compare October 6, 2022 01:48
@rfay
Copy link
Member

rfay commented Oct 6, 2022

Rebased again, another merge conflict. Applied @mattstein 's changes. I'll re-test tomorrow and we can get it in. Would appreciate if you could do manual test and review again @bbrala, thanks.

@bbrala
Copy link
Contributor Author

bbrala commented Oct 6, 2022

Tested it locally built from this branch.

  • ddev config drupal core.
  • ddev config global --project-tld lol.com
  • checked if it starts on right domain
  • ddev config --project-tld pap.com
  • checked if it starts on pap.com

It works :)

@rfay rfay merged commit c7c9b3f into ddev:master Oct 6, 2022
@rfay
Copy link
Member

rfay commented Oct 6, 2022

Thanks so much @bbrala ! Happy to have you do any of those other needed globals if you get inspired. Won't require so much rebasing next time, that was just about the timing of the big docs PR.

@rpkoller
Copy link
Collaborator

rpkoller commented Oct 6, 2022

wow nice addition! thanks a lot! i've already tested a few minutes ago by updating to the latest version in head. everything is working as expected. :) i had only one suggestion i brought up on discord and quickly chatted with @rfay about. currently the setting in the global config is project_tld: "" while in the config for project files you have# project_tld: ddev.site

i wonder if it would make sense to change project_tld: "" in ~/.ddev/global_config.yaml to project_tld: ddev.site that way it would be self explanatory which TLD ddev is using out of the box. the formatting would be also consistent with the project_tld used in project config files where no quotation marks are used (that inconsistency, for me as a none developer, was something that confused me. i didn't know that it doesn't matter if you use quotation marks as long as you dont have any spaces ). and by providing project_tld: ddev.site in the global config file it would also prevent the potential uncertainty in case i wanted to change the TLD if i need to prepend a period or not. that way everything would be self explanatory.

the only caveat with my suggestion @rfay pointed out on discord is that you have to be careful in case you are changing the global project_tld . ddev sets the default (project_tld: DdevDefaultTLD = "ddev.site") in https://github.com/drud/ddev/blob/af1918778c56f789568951437f0a7b6ed4b15752/pkg/nodeps/values.go#L109
and then compares the constant DdevDefaultTLD with ProjectTLD in https://github.com/drud/ddev/blob/5049bdfb0a2dadf1185c63972d2e238cd2120969/pkg/ddevapp/config.go#L103-L106

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.

5 participants