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

Add tests for Enso_Secrets, update to new cloud API #8736

Merged
merged 33 commits into from
Jan 15, 2024

Conversation

radeusgd
Copy link
Member

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@radeusgd radeusgd self-assigned this Jan 11, 2024
@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 11, 2024
@@ -16,7 +16,10 @@ public static String getToken() {

public static String getAPIRootURI() {
var envUri = Environment_Utils.get_environment_variable("ENSO_CLOUD_API_URI");
return envUri == null ? "https://7aqkn3tnbc.execute-api.eu-west-1.amazonaws.com/" : envUri;
var effectiveUri =
envUri == null ? "https://7aqkn3tnbc.execute-api.eu-west-1.amazonaws.com/" : envUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a constant

Copy link
Member Author

Choose a reason for hiding this comment

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

It couldn't, because we sometimes override the environment variable in tests. We would be unable to do so if it was baked-in at initialization time.

"2048")
.inheritIO()
.start()
.waitFor();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, it's good enough for testing

@@ -27,7 +27,7 @@ static String readSecret(String secretId) {
return secrets.get(secretId);
}

var apiUri = AuthenticationProvider.getAPIRootURI() + "/s3cr3tz/" + secretId;
var apiUri = AuthenticationProvider.getAPIRootURI() + "s3cr3tz/" + secretId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Security through obscurity?

- custom_ssl_context: A custom SSL context to use for requests, or
Nothing if the default should be used. For most use cases, it is
recommended to use the default.
Value timeout follow_redirects proxy version custom_ssl_context
Copy link
Member

Choose a reason for hiding this comment

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

Lets put the default on custom_ssl_context here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it is not needed, because this is an internal constructor - users should never use it anyway.

IMHO it is better to keep without the default, because:

  • if a library dev writes a new function that creates a modified HTTP instance, if there's a default for this property, they may forget to set this argument in the constructor, and such a new function will then be losing the ssl context settings by accident. Such situation may not be caught by tests, because it's a scenario unlikely to be tested (we don't usually test that all properties are retained after modifying one).
  • If there's no default, forgetting about this property will return a not-fully-applied constructor, which is a clear mistake and will be immediately caught as the function will just not work correctly.

So IMO it is safer to not have a default, because in most cases the context should be set to inherit the previous one when modifying the HTTP instance.

The only argument for having it would be for user convenience - but this constructor is not supposed to be used by users anyway - we have HTTP.new for that.

A helper method that retries the action a few times, to allow tests that may fail due to propagation delays to pass.
This is needed, because after creating a secret, there is a slight delay before it shows up within `list`.
To make tests robust, we add this retry logic.
with_retries ~action =
Copy link
Member

Choose a reason for hiding this comment

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

would this make sense in HTTP soon?
Limited retries and exponential back off?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I understand the question.

In HTTP tests or in HTTP user-facing API?

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jan 15, 2024
@mergify mergify bot merged commit f34abed into develop Jan 15, 2024
25 checks passed
@mergify mergify bot deleted the wip/radeusgd/8556-cloud-secrets-followup branch January 15, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow on from the Enso Secret APIs
4 participants