Skip to content

Conversation

@peb-adr
Copy link
Member

@peb-adr peb-adr commented Oct 10, 2023

No description provided.

@peb-adr peb-adr self-assigned this Oct 10, 2023
@peb-adr peb-adr marked this pull request as draft October 10, 2023 17:38
@peb-adr peb-adr requested a review from normanjaeckel October 24, 2023 13:47
Comment on lines 101 to 171
} else {
// Be compatible when given a single file
// -> mock an FS containing only that file
var err error
var content []byte
content, err = os.ReadFile(*tplDirName)
if err != nil {
return nil, fmt.Errorf("reading template file %q: %w", *tplDirName, err)
}
tplDirFs = fstest.MapFS{fInfo.Name(): {Data: content}}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

There may be a better way to do this.
From what I've read fstest.MapFS is actually intended for mocking files and dir trees for tests.
This is done to keep compatibility with providing a single file to --template

Comment on lines 206 to 273
if len(dirEntries) == 1 && !dirEntries[0].IsDir() {
var content []byte
content, err = fs.ReadFile(tplDirFs, dirEntries[0].Name())
if err != nil {
return fmt.Errorf("reading single template file %q: %w", dirEntries[0].Name(), err)
}
tplDirFs = fstest.MapFS{cfg.Filename: {Data: content}}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to the one above, but this is to keep compatibility with the filename config key.
When there is more than one file being templated filename will be ignored. Instead the output files will keep the names of the template files.

@peb-adr
Copy link
Member Author

peb-adr commented Oct 24, 2023

@normanjaeckel
Can you review this?

We are currently testing around with setting up OpenSlides on k3s. As with k3s deployment definitions are much more verbose, having them all in just one file (as currently done with the docker-compose.yml) would make it very unreadable and also not follow the best practices.
So I implemented the possibility to instead define the template as a directory containing go templates. The structure of that directory is then traversed and go templates are applied. The resulting files will then also be output following the same tree structure.
For this I utilized the FS package and it's FS abstractions. I think perhaps the most questionable changes are those made for backwards compatibility, also see my code comments for details.

@peb-adr peb-adr marked this pull request as ready for review October 24, 2023 14:03
content, err = fs.ReadFile(tplDirFs, dirEntries[0].Name())
if err != nil {
return "", err
return fmt.Errorf("reading single template file %q: %w", dirEntries[0].Name(), err)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably not good.
The filename config parameter should be obsoleted with these changes.
We may introduce a parameter like outputDirectory for resulting files after executing templates.

@normanjaeckel normanjaeckel marked this pull request as draft July 12, 2024 09:19
@normanjaeckel normanjaeckel marked this pull request as ready for review January 10, 2025 11:21
@normanjaeckel
Copy link
Member

We discussed this feature: The flag --technology should be renamed in --builtin-template. There should be a map with the possibilities like docker-compose, kubernetes with a nice map so that we can add more variants easily. If the --template flag is set, the --builtin-template is ignored or forbidden.

@normanjaeckel normanjaeckel removed their request for review January 11, 2025 22:34
@normanjaeckel
Copy link
Member

@peb-adr Please review.

Copy link
Member Author

@peb-adr peb-adr left a comment

Choose a reason for hiding this comment

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

Found some remnants of "technology"

Also while testing I found a few more details in behavior that seem to cause incompatibilities. I'll have to digest that further and will then comment them seperately

@normanjaeckel
Copy link
Member

@peb-adr All done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants