- Notifications
You must be signed in to change notification settings - Fork928
chore: generate countries.tsx from Go code#15274
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@johnstcn@dannykopping please do have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good from my end, some linter issues.
Could you add an exaple prompt comand (seecli/prompts.go)?
This will let us validate country selection more easily (e.g.go run ./cmd/coder exp prompt-example country)
cc@BrunoQuaresma for FE review
scripts/apitypings/main_test.go Outdated
| @@ -51,3 +52,36 @@ func TestGeneration(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
| func TestGenerateSliceOfObjectsTypeScript(t *testing.T) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| t.Parallel() |
scripts/apitypings/main.go Outdated
| countriesTS, err := generateSliceOfObjectsTypeScript("countries", codersdk.Countries) | ||
| if err != nil { | ||
| log.Fatal(ctx, fmt.Sprintf("generate countries typeScript: %s", err.Error())) | ||
| } | ||
| _, _ = fmt.Print("// The code below is generated from codersdk/countries.go.\n") | ||
| _, _ = fmt.Print(countriesTS, "\n") | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To do this correctly, we should not have to specifycodersdk.Countries. We should implement it here:
coder/scripts/apitypings/main.go
Lines 458 to 459 inafacb07
| case*types.Var: | |
| // TODO: Are any enums var declarations? This is also codersdk.Me. |
I feel a bit apprehensive to importcodersdk into this tool.
We could make it a new generator (:cry:) or actually auto-gen vars from the codersdk package (likecodersdk.Me).
If we do thevars, we might want to have some opt-in comment. Idk how many var declarations we have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should really have a general autogen tool and pattern for these. When the types are in Go and no package AST parsing is necessary.
rbacgen is an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
got it, will implement a general autogen tool. As suggested, I'm thinking of doing it similar to howrbacgen is done.
So the autogen tool will be underscripts/valuegen which containsmain.go which is the script itself andtypescript.tstmpl which will be the TS template to which we will convert thecountries.go
Line 3 in1702c64
| varCountries= []Country{ |
If similar types conversion needs are needed in the future for other types in Go, it can be handled in the script to use the sametypescriptconst.tstmpl template or a different one based on the need.
does that make sense? wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@joobisb That all sounds reasonable. It'll probably throw into a different.ts file.
It would be awesome ifrbacgen andvaluegen could be merged into 1 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Emyrk I have updated the scripts and have merged both into a singletypegen script, please do have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perfect 👍
Head branch was pushed to by a user without write access
@matifali could you please help to run the CI again and merge, I've fixed a formatting error |
I am unsure why that premium e2e is failing. I'll take a quick look |
@Emyrk Because PRs from forks do not have access to secrets. |
matifali commentedNov 13, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This could be solved by using, on:pull_request_target: The only downside is that the workflows are run from the target branch, but the code is checked out from the PR branch. |
@joobisb Thanks for the reminder. I will try something like the one below and let you know if it works.
|
4cb8076 intocoder:mainUh oh!
There was an error while loading.Please reload this page.
Closes#15074
We have a hard-coded list of countries athttps://github.com/coder/coder/blob/main/site/src/pages/SetupPage/countries.tsx. This means Go code in coder/coder doesn't have an easy way of utilizing it.
Solution
Generate countries.tsx from Go code. Generated by
scripts/apitypings