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

Profile mutation and serialization #2818

Conversation

jonathanmorley
Copy link

Motivation and Context

Supports mutating ProfileSets, so that applications that fetch STS credentials can update a ProfileSet with the new credentials.

Supports serializing ProfileSets, so that those credentials can be written back to disk.

Description

Testing

New test added to src/profile/parser.rs

Checklist


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jonathanmorley jonathanmorley requested a review from a team as a code owner June 28, 2023 21:24
@jonathanmorley jonathanmorley changed the title Profile mutation serialization Profile mutation and serialization Jun 28, 2023
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

I think ultimately, this isn't going to be the right approach. A full round trip of the file is probably not going to be what users want—I think we will almost certainly want to append to the file instead. That way we can preserve comments, keep the users profile in order, and not worry about unsupported features.

For that reason, I think the thing I'd add serializability for is a profile itself. That also removes the need to make ProfileSet mutable.

When parsing profiles, we are able to keep track of line number information. If we include that, you could even update your profile by tracking the offsets.

I don't think we'll be able to merge this PR as is unfortunately for the reasons above.

@@ -114,6 +114,11 @@ impl ProfileSet {
self.profiles.get(profile_name)
}

/// Set a named profile in the profile set
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add docs that clarify that this will replace another profile with the same name. Might as well change the API to return the removed profile if it existed as well

Comment on lines +137 to +138
/// Returns a string representation of the profiles, with syntax based on `kind`.
pub fn to_string(&self, kind: ProfileFileKind) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll need a few caveats in this method:

  1. It is going to delete any comments the user may have had
  2. We need to test how it handles multi-line values, e.g. for S3:
[profile development]
s3 =
  max_concurrent_requests = 20
  max_queue_size = 10000
  multipart_threshold = 64MB
  multipart_chunksize = 16MB
  max_bandwidth = 50MB/s
  use_accelerate_endpoint = true
  addressing_style = path

@rcoh
Copy link
Collaborator

rcoh commented Aug 3, 2023

closing this for now—let us know (or submit a PR) if you'd like to make some more public methods

@rcoh rcoh closed this Aug 3, 2023
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