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

fix missing server configs on hasura and towel deployments #346

Merged
merged 5 commits into from
Feb 25, 2022

Conversation

gabriel-milan
Copy link
Contributor

@gabriel-milan gabriel-milan commented Feb 24, 2022

Summary

As I've spoken to @kvnkho on Slack, the Prefect Server configurations set on the values.yaml file are still missing on both Hasura and Towel deployments.

Importance

This fixes misconfiguration for Hasura and Towel.

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)

@zanieb
Copy link
Contributor

zanieb commented Feb 24, 2022

Hi! We can add this to the towel service, which indeed does consume some of the config values for Prefect Server. However, Hasura does not use our config so I don't see what including the environment variables there would do. Is there a specific issue that's resolving for you? (from the slack thread it seems like just towel needs the update)

@gabriel-milan
Copy link
Contributor Author

That's correct. Only Towel needs the update for my use case. I just thought that maybe it was also missing for Hasura, but if it's not used, I think that it's fine.

@zanieb
Copy link
Contributor

zanieb commented Feb 25, 2022

That's correct. Only Towel needs the update for my use case. I just thought that maybe it was also missing for Hasura, but if it's not used, I think that it's fine.

Yeah their container won't do anything with those variables so we probably shouldn't include them.

@gabriel-milan
Copy link
Contributor Author

we probably shouldn't include them

I've just removed it, thanks for saying that!

@zanieb
Copy link
Contributor

zanieb commented Feb 25, 2022

@gabriel-milan thanks! Looks like you'll just need to get up to date with the master branch now.

@gabriel-milan
Copy link
Contributor Author

does it seem correct? i'm not sure

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Yep! Thanks!

@zanieb zanieb merged commit 48fd3ee into PrefectHQ:master Feb 25, 2022
@zanieb zanieb mentioned this pull request Mar 29, 2022
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.

None yet

2 participants