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

[Storage] MatchCondition instead of various access condition #5466

Closed
jeremymeng opened this issue Oct 8, 2019 · 12 comments · Fixed by #5672
Closed

[Storage] MatchCondition instead of various access condition #5466

jeremymeng opened this issue Oct 8, 2019 · 12 comments · Fixed by #5672
Assignees
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)

Comments

@jeremymeng
Copy link
Member

jeremymeng commented Oct 8, 2019

Related discussion at Azure/azure-sdk#638

Looks we are going to have some flags

ifChanged/ ifUnchanged

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 8, 2019
@triage-new-issues triage-new-issues bot removed the triage label Oct 8, 2019
@jeremymeng jeremymeng added the blocking-release Blocks release label Oct 10, 2019
@jeremymeng
Copy link
Member Author

This is only for etag

@HarshaNalluru
Copy link
Member

Reach out to @richardpark-msft

@richardpark-msft
Copy link
Member

I think the difference between how we're doing in appconfig and how the rest of the world is doing it is that since our model object (ConfigurationSetting) is always passed as an argument we can take the etag value directly from it rather than requiring the user to pass it separately.

You can see that here they can just pass in the setting:

const actualSetting = await client.getConfigurationSetting({ key: key });

Then to "activate" it they just specify a boolean parameter:

await client.setConfigurationSetting(actualSetting, {
onlyIfUnchanged: true
});

In the etag discussion yesterday I believe that Ted mentioned that storage couldn't necessarily do that because the object that gets passed in doesn't have an etag field.

So I'm not sure if what I did would apply to what you're doing in storage.

For completeness here's the section in the design guide that talks about conditional requests:
https://github.com/Azure/azure-sdk/blob/master/docs/general/design.md#conditional-requests

@bterlson
Copy link
Member

I believe the intention of the guidelines is that the condition option with the onlyIfChanged set of options are simple options when an entity is passed in with an etag. For storage, this is not the case, so a more general purpose option type is needed. For that type, I think using the standard etag names as storage is already using (ifMatch/ifNoneMatch) is appropriate, and their values are strings.

One thing I'm not sure about: do we still call the opting conditions but its type is a ModifiedAccessConditions rather than the SimpleConditions with boolean flags? /cc @tg-msft in case you know what .net's plans are here :)

@tg-msft
Copy link
Member

tg-msft commented Oct 17, 2019

Azure/azure-sdk-for-net#8138 to cross reference what we're doing in .NET

@HarshaNalluru
Copy link
Member

@jeremymeng, Looks like storage JS doesn't require any changes. If so, can we close this issue?

@jeremymeng
Copy link
Member Author

We are pretty close to .NET has currently except names. At this point I'd want to keep the old names.

@richardpark-msft
Copy link
Member

Chatted with @annelo-msft - AppConfig in .net is NOT exposing MatchConditions (or it's cousin RequestConditions) for AppConfig. It's all done via the bool onlyIfChanged parameter.

@bterlson
Copy link
Member

I think no change is required here, so closing.

@bterlson
Copy link
Member

I am possibly wrong. Discussing more tomorrow.

@jeremymeng
Copy link
Member Author

Blob: #5672
File: #5682

@ramya-rao-a
Copy link
Contributor

Closing as #5672 and #5682 are complete

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants