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

don't validate endpoint config with --secret #315

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

fauna-chase
Copy link
Contributor

ENG-5661
This came up while I was testing updating schema in a pipeline. In this scenario the endpoints present in the project configuration won't exist, which is ok as long as a secret is getting passed.

ENG-5661
This came up while I was testing updating schema in a pipeline.  In this
scenario the endpoints present in the project configuration won't exist,
which is ok as long as a secret is getting passed.
@fauna-chase fauna-chase requested a review from macmv October 18, 2023 00:50
@@ -250,7 +248,7 @@ export class ShellConfig {
);
}

if (endpointName === undefined) {
if (endpointName === undefined || secretFlag !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you pass --secret, and endpointName is set, we need to pull the url and graphqlHost from that endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a problem for our CD use case, there will be an endpoint set from the configuration file which sets the endpointName. so I guess what I'm looking for here is secret is set and the absence of a root config.

Copy link
Contributor

Choose a reason for hiding this comment

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

right. so this change looks good, just the below part that says url: urlFlag should instead be url: urlFlag ?? endpoint.url or something.

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 that makes sense, but really I am targeting an empty root config scenario here, at least that was my intent, so I updated the check to account for that, lmk what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good to me

@fauna-chase fauna-chase force-pushed the secret-flag-no-endpoint branch from 304c26d to 6a237de Compare October 18, 2023 20:36
@fauna-chase fauna-chase merged commit 6d1ecfb into main Oct 18, 2023
@fauna-chase fauna-chase deleted the secret-flag-no-endpoint branch October 18, 2023 20:38
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.

2 participants