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

feat(proxy-sigv4-backend): add region and service options #19

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

jsecchiero
Copy link
Contributor

expose region and service from aws4, reference to that options here
example:

proxysigv4:
  '/aws-amp':
    target: https://aps-workspaces.us-west-2.amazonaws.com
    region: us-west-2
    service: aps

@alecjacobs5401
Copy link
Contributor

Thanks for the contribution @jsecchiero!

This looks great to me, though the build is failing for prettier on the README changes. Once you resolve I'll happily approve and merge

@jsecchiero
Copy link
Contributor Author

done!

@segmentio segmentio deleted a comment from seg-atlantis-prod bot Oct 21, 2024
@segmentio segmentio deleted a comment from seg-atlantis-prod bot Oct 21, 2024
@segmentio segmentio deleted a comment from seg-atlantis-prod bot Oct 21, 2024
@segmentio segmentio deleted a comment from seg-atlantis-prod bot Oct 21, 2024
Copy link
Contributor

@alecjacobs5401 alecjacobs5401 left a comment

Choose a reason for hiding this comment

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

Looks like some extra files were added in the latest commit, do you mind removing those to avoid any confusion?

Also, we do need to update the actual config.d.ts file for the new service and region configuration, I missed that initially (my bad)

Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to commit this?

Copy link
Contributor Author

@jsecchiero jsecchiero Oct 21, 2024

Choose a reason for hiding this comment

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

no i just used it locally and added it erroneously in the last rebase, sorry!
removed now

Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to commit this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

Comment on lines 18 to 32
| {
/**
* Target of the proxy. Url string to be parsed with the url module.
*/
target: string;
/**
* Use credentials for this IAM Role ARN via STS:AssumeRole call when signing the request.
*/
roleArn?: string;
/**
* Use this session name when performing the STS:AssumeRole call for roleArn. Default: 'tempAssumeRoleSession'.
*/
roleSessionName?: string;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to be adding the pkg directory here, but this does remind me that we should be updating the actual config.d.ts file for these new options, sorry for the late catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@alecjacobs5401
Copy link
Contributor

If we can update the config.d.ts file for the new config options, then this is good to go! Thanks

@jsecchiero
Copy link
Contributor Author

done!

@alecjacobs5401 alecjacobs5401 merged commit d05d196 into segmentio:main Oct 23, 2024
5 checks passed
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