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

Draft PR for Snapshots #22069

Closed
wants to merge 6 commits into from
Closed

Draft PR for Snapshots #22069

wants to merge 6 commits into from

Conversation

MBSolomon
Copy link
Contributor

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

@github-actions github-actions bot added the App Configuration Azure.ApplicationModel.Configuration label Nov 29, 2023
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

First pass.


return ListSnapshotsPagerResponse{
Snapshots: ss,
SyncToken: SyncToken(*page.SyncToken),
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for SyncToken to be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The service will provide a Sync-Token header in each response: https://learn.microsoft.com/en-us/azure/azure-app-configuration/rest-api-consistency#response

@jhendrixMSFT
Copy link
Member

LRO recordings blocked on #22102

@@ -76,3 +76,119 @@ type SetSettingOptions struct {
// if the passed-in ETag is the same version as the one in the configuration store.
OnlyIfUnchanged *azcore.ETag
}

type BeginCreateSnapshotOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

Missing doc comment (applies to other new types in this file).

Created *time.Time

// READ-ONLY; A value representing the current state of the snapshot.
Etag *string
Copy link
Member

Choose a reason for hiding this comment

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

This should be named ETag and use the type from azcore.

@@ -78,3 +79,70 @@ type SetSettingResponse struct {
// SyncToken contains the value returned in the Sync-Token header.
SyncToken SyncToken
}
type ArchiveSnapshotResponse struct {
Copy link
Member

Choose a reason for hiding this comment

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

Missing doc comment (applies to other new response types in this file).

@@ -78,3 +79,70 @@ type SetSettingResponse struct {
// SyncToken contains the value returned in the Sync-Token header.
SyncToken SyncToken
}
type ArchiveSnapshotResponse struct {
generated.Snapshot
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to directly expose the generated type. I presume we'll want to define a Snapshot type (similar to how we did for Setting) so it can have the shape we want.

Link *string

// SyncToken contains the information returned from the Sync-Token header response.
SyncToken *string
Copy link
Member

Choose a reason for hiding this comment

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

Since a response always contains a sync token, these don't need to be pointer-to-type.

generated.Snapshot

// Link contains the information returned from the Link header response.
Link *string
Copy link
Member

Choose a reason for hiding this comment

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

What's in the link header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something in this format:

"</kv?snapshot=&api-version=2023-10-01>; rel="items""

Copy link
Member

Choose a reason for hiding this comment

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

internal@v1.5.1 is now available. You can pick that up and rebase so these changes evaporate.

@jhendrixMSFT
Copy link
Member

Closing in favor of #22118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants