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

Fixes #81 - wrong app var name for python expose #82

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

DavidSeptimus-Klotho
Copy link
Contributor

@DavidSeptimus-Klotho DavidSeptimus-Klotho commented Jan 13, 2023

Fixes #81 by changing the AppVarName value set in the gateway spec back from Capability.ID to the name of the exported variable associated with the annotated FastAPI app.

compiled output for py-microservices Output after change:
image
image
image

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?

h.RootPath = app.RootPath

gwSpec := gatewaySpec{
FilePath: f.Path(),
AppVarName: cap.ID,
AppVarName: appVarName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to break the resource validation though. iirc, the resource validation is looking for a gateway with a given gateway ID as the name

Copy link
Contributor

Choose a reason for hiding this comment

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

So that might have been why this became an issue, the name was changed to fix the resource validation issue but then caused a regression w/ proxy calls

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah same comment as below

h.RootPath = app.RootPath

gwSpec := gatewaySpec{
FilePath: f.Path(),
AppVarName: cap.ID,
AppVarName: appVarName,
Copy link
Contributor

Choose a reason for hiding this comment

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

isnt this going to result in validation to fail again. On line 130 we call

		gw := core.NewGateway(spec.AppVarName)

so then wont this not match our ID? other gatewayspecs have an id field so we can track it so i dont think this prevents the warning from popping up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I just went back and added gatewayId to the spec and updated this line to use that value.

@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 2%
github.com/klothoplatform/klotho/pkg/annotation 24%
github.com/klothoplatform/klotho/pkg/core 20%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 45%
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 47%
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 60%
github.com/klothoplatform/klotho/pkg/runtime 75%
github.com/klothoplatform/klotho/pkg/static_unit 32%
github.com/klothoplatform/klotho/pkg/validation 73%
Summary 42% (3592 / 8587)

@DavidSeptimus-Klotho DavidSeptimus-Klotho merged commit 879a394 into main Jan 13, 2023
@DavidSeptimus-Klotho DavidSeptimus-Klotho deleted the py-appvarname branch January 13, 2023 16:21
atorres-klo pushed a commit that referenced this pull request Aug 14, 2024
Add extra dryrun level for tsc only
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.

Python expose plugin is substituting the capability in place of the FastAPI app variable name
3 participants