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

Update SDK for new deployment spec #97

Merged
merged 11 commits into from
Feb 16, 2024
Merged

Update SDK for new deployment spec #97

merged 11 commits into from
Feb 16, 2024

Conversation

paf31
Copy link
Collaborator

@paf31 paf31 commented Feb 13, 2024

  • Use new environment variable names and defaults
  • Remove configuration server
  • Pass PathBuf to validate_raw_configuration and remove RawConfiguration
  • Update Dockerfile to meet the deployment spec

Dockerfile Outdated Show resolved Hide resolved
rust-connector-sdk/src/connector.rs Outdated Show resolved Hide resolved
rust-connector-sdk/src/default_main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@SamirTalwar SamirTalwar left a comment

Choose a reason for hiding this comment

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

Approving so it's not blocked, but please consider my comments.

rust-connector-sdk/src/default_main.rs Outdated Show resolved Hide resolved
long,
value_name = "DIRECTORY",
env = "HASURA_CONFIGURATION_DIRECTORY",
default_value = "/etc/connector"
Copy link
Collaborator

@daniel-chambers daniel-chambers Feb 15, 2024

Choose a reason for hiding this comment

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

I'm not sure how easy this is to do, but is it possible to remove this default when compiled for Windows? I know we don't care about deployment on Windows, but theoretically Windows developers may build a connector using this SDK and when they run it locally, it should not default to an impossible path, instead we should prefer to just make them specify this arg manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also applies to the other commands that have this same parameter: ServeCommand, ReplayCommand

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I think this point is bigger than just Windows vs. Unices.

In a Linux Docker image, we want to read /etc/connector. But when running locally for testing, we want to read the specified directory; we almost never want to use /etc/connector, even on Linux.

I think the right move here is to remove the default value and set ENV HASURA_CONFIGURATION_DIRECTORY /etc/connector in connector Docker images. This can be overridden.

Long story short, the default is a packaging concern, not an SDK concern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I think about it, I agree. I'll remove the default and move it to the Dockerfile. For reference, it was possible though:

#[cfg_attr(target_family = "unix", arg(default_value = "/etc/connector"))]

@daniel-chambers daniel-chambers mentioned this pull request Feb 15, 2024
2 tasks
@SamirTalwar
Copy link
Contributor

Let's merge this. Gonna follow up with a change to an error enum but that needs its own review.

@SamirTalwar SamirTalwar merged commit 660750a into main Feb 16, 2024
5 checks passed
@SamirTalwar SamirTalwar deleted the deployment-spec-sdk branch February 16, 2024 13:40
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.

3 participants