Allow directory names with hyphens in _dev directories #270
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
jsoriano
force-pushed
the
ignore-validation-dev-directories
branch
from
07eaa21 to
b1b7c6e
Compare
| @@ -85,7 +85,7 @@ func (s *folderSpec) validate(packageName string, folderPath string) ve.Validati | |||
| if itemSpec == nil && s.AdditionalContents { | |||
| // No spec found for current folder item but we do allow additional contents in folder. | |||
| if file.IsDir() { | |||
| if strings.Contains(fileName, "-") { | |||
| if !s.DevelopmentFolder && strings.Contains(fileName, "-") { | |||
There was a problem hiding this comment.
A short challenge:
Do you agree that it is unusual to forbid - in dirnames? If so, maybe we should forbid it only for specific Kibana/Elasticsearch items. For example, I don't think that - should be forbidden for subdirectories in docs or images, or any other extra content, the developer would like to put there.
Long story short - instead of introducing DevelopmentFolder flag, I would consider a StackInstalledFlag, which forbids dashes.
Let me know what you think.
There was a problem hiding this comment.
Do you agree that it is unusual to forbid
-in dirnames?
Yeah, I fully agree :)
But if we forbid them I think it'd be less confusing to forbid them everywhere (as proposed in elastic/package-registry#517) instead of on specific directories.
The exception in development directories (or at least in deploy directories) can make sense because there can be files there that have hyphens by convention in other systems (as the hyphens to separate words in some files in docker images).
Btw, if we allow hyphens everywhere except for directories containing stack files, we have to reconsider the removal of the check in the package registry that originated #259.
There was a problem hiding this comment.
But if we forbid them I think it'd be less confusing to forbid them everywhere (as proposed in elastic/package-registry#517) instead of on specific directories.
I admit I follow the intuition here and it would be confusing no matter which option we follow. I'm rather thinking about what's better for future use cases. I can imagine extra directories with docs, resources, extra scripts for testing.
Btw, if we allow hyphens everywhere except for directories containing stack files, we have to reconsider the removal of the check in the package registry that originated #259.
Yeah, I'm thinking about this. Maybe this is the right timing to adjust it to match only relevant directories.
Anyway, back to this PR. We have only development resources, which are affected by this, so I would say it's fine to push this one and come back to the discussion when somebody else tries to a folder with a hyphen.
There was a problem hiding this comment.
Maybe this is the right timing to adjust it to match only relevant directories.
I would remove instead of adding validation logic from the registry 🙂
There was a problem hiding this comment.
Well, we have spec now, so maybe we can make the Package Registry a bit more stupid :)
| @@ -1,5 +1,6 @@ | |||
| spec: | |||
| additionalContents: false | |||
| developmentFolder: true | |||
There was a problem hiding this comment.
nit: _dev is also in data_stream. You may add a test case that verifies that one too.
There was a problem hiding this comment.
Good catch, for some reason I thought they shared the same spec. Fixed.
Add new flag
developmentFolderto indicate if a directory contains development files. Inside these directories it is allowed to have files with hyphens.Follow up to #259.
Solves issue mentioned in #259 (comment):