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

make fileref use root path and stat unit have same logic as embed #34

Merged
merged 2 commits into from
Jan 6, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

• Does any part of it require special attention?
• Does it relate to or fix any issue?

Since file ref needs to read the actual code from the file we need to supply it with the path from our config to join to the file path.

Also changing static unit to match files based on relative path and absolute path to close #33

This is all work for the search the deck changes

Standard checks

  • Unit tests: Any special considerations?
  • Docs: Do we need to update any docs, internal or public?
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working?

@@ -98,6 +105,45 @@ type staticAssetPathMatcher struct {
err error
}

func (m *staticAssetPathMatcher) ModifyPathsForAnnotatedFile(path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you doc a bit what this function does? I'm getting lost in the "Abs", "Rel" etc. It likely is similar to how readdir works, but I've forgotten how that works 😄

var f core.File = &core.FileRef{FPath: relPath}
var f core.File = &core.FileRef{
FPath: relPath,
RootConfigPath: cfg.Path,
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to be mindful of here is that this path is relative to the "root", which is split from the original passed-in path in splitPathRoot. This is to prevent plugins from accessing arbitrary files (eg, FPath: "../../../etc/passwd").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when do we allow access to files outside of the root of the klotho run? Im failing to understand this a little bit so do you have a scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was more a defense against mistakes and third-party plugins (when we get them). Might not be necessary for now, but it does mean this could be incorrect - try doing klotho ../ --app blah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, let me make a note of that and run through that scenario, if it doesnt work, ill cut a card to fix it

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 2%
github.com/klothoplatform/klotho/pkg/annotation 24%
github.com/klothoplatform/klotho/pkg/core 21%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 41%
github.com/klothoplatform/klotho/pkg/infra/kubernetes 58%
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 52%
github.com/klothoplatform/klotho/pkg/input 63%
github.com/klothoplatform/klotho/pkg/lang 37%
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0%
github.com/klothoplatform/klotho/pkg/lang/golang 9%
github.com/klothoplatform/klotho/pkg/lang/javascript 46%
github.com/klothoplatform/klotho/pkg/lang/python 60%
github.com/klothoplatform/klotho/pkg/lang/yaml 0%
github.com/klothoplatform/klotho/pkg/logging 7%
github.com/klothoplatform/klotho/pkg/multierr 95%
github.com/klothoplatform/klotho/pkg/provider/aws 54%
github.com/klothoplatform/klotho/pkg/runtime 75%
github.com/klothoplatform/klotho/pkg/static_unit 32%
github.com/klothoplatform/klotho/pkg/validation 73%
Summary 42% (3554 / 8547)

Copy link
Contributor

@ewucc ewucc left a comment

Choose a reason for hiding this comment

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

nothing blocking from me, just that question and the docs gordon mentioned

if err != nil {
return err
}
if absPath == pattern {
Copy link
Contributor

Choose a reason for hiding this comment

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

im not quite understanding why this is a special case. Won't every absPath have a /? Why is the prefix only trimmed if the filpath.Abs(pattern) seemingly does nothing to the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so our paths for files are like src/somefile.txt

if they specify an absolute path of /src/somefile.txt we wont match unless we remove the prefix /

The other scenario after that is for relative paths, which wont start with / anyways.

The only real reason we check absPath == pattern is just to make sure its a true absolute path.

If that doesnt make sense we can rework this or have another discussion, going to push to get the search the deck stuff going for ala

@jhsinger-klotho jhsinger-klotho merged commit 1251f55 into main Jan 6, 2023
@jhsinger-klotho jhsinger-klotho deleted the file_ref branch January 6, 2023 00:54
atorres-klo pushed a commit that referenced this pull request Aug 14, 2024
* Cleans up error handling in the up command

* Returns an error if the "Name" input is supplied

* Clarifies default region error

* Adds utilities for some frequent reflection operations
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.

make static unit follow the same paths in matching as embed assets
3 participants