Skip to content
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

feat(modulegen): print out VSCode workspace file if needed #1549

Merged
merged 14 commits into from
Aug 29, 2023

Conversation

mdelapenya
Copy link
Member

@mdelapenya mdelapenya commented Aug 29, 2023

What does this PR do?

Following the recent refactors, it's now very easy to extend the module generator to generate more things. In this PR we are generating the VSCode workspace file, but instead of generating a file that could live whenever the user wants, it will generate the workspace file into the .vscode directory of the repository. This dir is ignored by git, so it won't impact in other users when the file is generated including absolute paths.

To achieve this, we have adapted how our internal templating engine works: it accepted a string with the path where to write the file, in this PR we are adding a new function to also accept an io.Writer reference: Generate will use the Writer, and GenerateFile will use the string file, calling back to Generate once a File is created (os.File implements io.Writer).

Before this PR, there were a few calls to this generation, so we have updated the calls to use the new GenerateFile function.

The generation of the VSCode template happens at the end of the generation.

Why is it important?

I see myself many times refreshing the VSCode workspace to see all modules in one single VSCode project, and I do this manually. With the current refactors (big kudos to @mmorel-35!!!) now it's easy to extend the generator with a few lines of code, and this improvement was very cheap in time and code.

@mdelapenya mdelapenya requested a review from a team as a code owner August 29, 2023 12:54
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Aug 29, 2023
@mdelapenya mdelapenya self-assigned this Aug 29, 2023
@netlify
Copy link

netlify bot commented Aug 29, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 0cf8341
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64ee170537b65e00083f2ab7
😎 Deploy Preview https://deploy-preview-1549--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Member Author

@mmorel-35 could you please take a look?

func Generate(t *template.Template, exampleFilePath string, name string, data any) error {
err := os.MkdirAll(filepath.Dir(exampleFilePath), 0o755)
if err != nil {
return err
}
exampleFile, _ := os.Create(exampleFilePath)
defer exampleFile.Close()

err = t.ExecuteTemplate(exampleFile, name, data)
func Generate(t *template.Template, wr io.Writer, name string, data any) error {
err := t.ExecuteTemplate(wr, name, data)
Copy link
Contributor

@mmorel-35 mmorel-35 Aug 29, 2023

Choose a reason for hiding this comment

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

What do you think of having Two functions instead

func GenerateFile(t *template.Template, exampleFilePath string, name string, data any) error {

And

func Generate(t *template.Template, wr io.Writer, name string, data any) error {

This way you keep inside GenerateFile the logic of creation of directories and file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea. Let me rework on this PR

@mmorel-35
Copy link
Contributor

General question.
Why not generate it in a file in the project as there is already a folder for .devcontainer ?

@mdelapenya
Copy link
Member Author

Why not generate it in a file in the project as there is already a folder for .devcontainer ?

I did not want to couple it with VSCode, but you are right: we already have a devcontainer file. Do you think generating it in the root folder with a hidden name is fine? Something like .testcontainers-go.code-workspace

@mmorel-35
Copy link
Contributor

Another question.
Once you can split commands, you probably wants to list them first.
modulegen is actually generating the whole module from the root command
What do you expect to see ?

  • modulegen newModule
  • modulegen printVSCodeWorkspace
    ...

@mmorel-35
Copy link
Contributor

mmorel-35 commented Aug 29, 2023

Something like .testcontainers-go.code-workspace

What are the practices recommended by vscode. From what I could see they usually define a dedicated folder in the repository.

Edit:
After reading the link you provided, the naming you are proposing seems to be a good choice.

@mdelapenya
Copy link
Member Author

What do you expect to see ?

At some point I'd like to have one command per file that is not for the module itself, said in other words: a shared file: ci.yml, mkdocs, dependabot, vscode workspace... So I want to be able to refresh them and put it on sync

@mdelapenya
Copy link
Member Author

mdelapenya commented Aug 29, 2023

@mmorel-35 I'm thinking about the content of the VSCode file:

{
    "path": "src/github.com/testcontainers/testcontainers-go/examples/datastore"
},

The src/github.com part is specific to my machine and where I have the repository. Don't you think that calculating that base path for all users would not be accurate? We could get the current execution dir and from there browse the parent file system until we get to github.com's parent. But could that be flaky?

@mmorel-35
Copy link
Contributor

mmorel-35 commented Aug 29, 2023

Would it work if the path was relative to the file ?
But I'm worried it would not work on Windows with the different path separator.
Maybe this file needs to not be committed to the repository. And
With the use of cobra a new command can be created in modulegen
something like go run . createCodeWorkspace

You can even complete the gitignore do it's never committed.

@mdelapenya
Copy link
Member Author

We can keep it under the .vscode dir, which is already ignored. But I'm concerned about the motivation for generating an ignored file. Would it be useless?

@mmorel-35
Copy link
Contributor

The file is useful for anyone who wants to develop on this project. But as you said it, depending on the platform and the users setup the file path for the modules will be different. So modulegen can provide a way to generate the file but needs to ensure that you don't receive the config file of somebody else .

@mdelapenya
Copy link
Member Author

The file is useful for anyone who wants to develop on this project. But as you said it, depending on the platform and the users setup the file path for the modules will be different. So modulegen can provide a way to generate the file but needs to ensure that you don't receive the config file of somebody else .

I think I have it: thanks to the existing building blocks, I can get the root dir, retrieve its absolute file path and pass it to the template, for all modules. Using filepath will guarantee the cross-platform thing.

If you like the implementation, I'll update the description to reflect the discussion in here

Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing those tests ?

Copy link
Member Author

@mdelapenya mdelapenya Aug 29, 2023

Choose a reason for hiding this comment

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

Because they were too coupled to the implementation, and having to update them with every change "smelled to me". In fact, there are many of this type of tests in the module generator TBH

Copy link
Contributor

@mmorel-35 mmorel-35 left a comment

Choose a reason for hiding this comment

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

It LGTM !
You can update the description with what we discussed.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mdelapenya
Copy link
Member Author

It LGTM ! You can update the description with what we discussed.

Updated! Thanks for your review and guidance through this refactor, lots of learning!

@mdelapenya mdelapenya merged commit 372c550 into testcontainers:main Aug 29, 2023
@mdelapenya mdelapenya deleted the generate-vscode-workspace branch August 30, 2023 12:05
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 30, 2023
* main: (32 commits)
  fix: remove wrong example from workspace (testcontainers#1556)
  chore(deps): bump the all group in /modules/localstack with 1 update (testcontainers#1552)
  modulegen: generate code-workspace with json marshal (testcontainers#1551)
  chore(deps): bump the all group in /modules/compose with 2 updates (testcontainers#1553)
  feat: add mariadb module (testcontainers#1548)
  feat(modulegen): print out VSCode workspace file if needed (testcontainers#1549)
  modulegen: generate md file inside internal/mkdocs (testcontainers#1543)
  modulegen: create internal/module and internal/modfile (testcontainers#1539)
  [Enhancement]: add ability to set repo:tag for ContainerRequest FromDockerfile (testcontainers#1508)
  Fix module generator for examples (testcontainers#1545)
  Update pulsar.md (testcontainers#1542)
  modulegen: create internal/make (testcontainers#1537)
  chore: fix workflow (testcontainers#1538)
  chore(deps): bump the all group in /examples/cockroachdb with 1 update (testcontainers#1522)
  chore(deps): bump the all group in /examples/bigtable with 1 update (testcontainers#1534)
  chore(deps): bump the all group in /modules/localstack with 4 updates (testcontainers#1535)
  chore(deps): bump the all group in /modules/k3s with 2 updates (testcontainers#1526)
  chore(deps): bump the all group in /examples/spanner with 2 updates (testcontainers#1532)
  chore(deps): bump the all group in /examples/firestore with 1 update (testcontainers#1523)
  chore(deps): bump the all group in /modules/redis with 1 update (testcontainers#1524)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants