Updated to document breaking changes

[MaryanneNjeri]

This PR updates the AzureDevOpsPushPipelineExtenstion.md and documents importing feature flag breaking change
GitHub issue #760

[zhenlan]

This is a breaking change too.

[MaryanneNjeri]

Yes, updated to document the breaking change.

[zhenlan]

Is it true? I thought it was prohibited.

[MaryanneNjeri]

Sorry my bad, it's prohibited, we throw an error to let user know.

[RichardChen820]

Remove extra whitespace

[MaryanneNjeri]

Updated and removed extra whitespace

[jimmyca15]

I'd recommend having a single breaking changes section with bullet points below of the breaking changes.

Breaking Changes

• Updated the task to require Node.js 16. It previously required 10.
• Updated the minimum supported agent version to 2.206.1. Previously it was 2.144.0

[MaryanneNjeri]

Updated to have a single breaking changes section with bullet points.
To confirm for the Pull task since we only document one breaking change ie on the Node js upgrade, we don't need to have it in bullet points and can have the before and after sections?

[jimmyca15]

The before and after is a bit hard to follow. I'd like if we can be a bit more direct on what's broken since these are often used for reference. It sounds like maybe it should be:

• Feature flag import now requires adherence to the feature management schema. Settings with feature flag prefix and content type will fail to import.

---

A follow up question here. Why did this break?

[MaryanneNjeri]

Updated to have the Breaking changes with bullet points and removed the before and after.

[MaryanneNjeri]

In version 5 of the Push task, we integrated with the sync lib, which tries to have the import behavior consistent with the CLI, the CLI logs a warning on Ignoring the key when importing kvs, and key starts with feature flag prefix, https://github.com/Azure/azure-cli/blob/fde406f3aa731d33e08ef7a77404b25f10f3b559/src/azure-cli/azure/cli/command_modules/appconfig/_kv_helpers.py#L108

[jimmyca15]

The CLI logs a warning, yet this fails outright? Doesn't seem like consistent behavior.

[MaryanneNjeri]

Since Sync client lib doesn't take a logger object, all warnings are treated as errors in the Sync lib. Though I remember on the Sync client lib road map we plan to support a common logger object Pull Request 7654083: Add custom logger support

[jimmyca15]

The portal doesn't allow creation of feature flags as raw KVs.

Push task doesn't allow it (our current discussion)

but CLI does allow it.

I think first we need to align on what we want the behavior to be across all client side tools.

[MaryanneNjeri]

The Portal supports importing configurations from a file and specifying the prefix as ".appconfig.featureflag/" and content type "application/vnd.microsoft.appconfig.ff+json;charset=utf-8"

[jimmyca15]

But the portal doesn't allow that in the individual kv create flow. Right?

[MaryanneNjeri]

Yes right

[jimmyca15]

This is a change for all files using application/json content type right, not just kv set? If so, we'll want to call that out.

Also, may help to provide a snippet of what has changed.

Something like

Before when you had this

bla bla

you got this

bla bla bla

Now, the input needs to be

bla bla bla

[MaryanneNjeri]

It only applies for default content profile, using application/json content type.
How about something like :
"Importing configurations with values as escaped json strings and the content type "application/json" is no longer supported. Please specify value as a json object."

Before

{  
  "app": "{\"uri\":\"https://keyvault.vault.azure.net/secrets/secret\"}"
}

Results:

Key	Value
app	{"uri":"https://keyvault.vault.azure.net/secrets/secret"}

After

{  
  "app": {
      "uri": "https://keyvault.vault.azure.net/secrets/secret " 
     }
}

[MaryanneNjeri]

I was thinking since the breaking change is in relation to importing configurations, we can show the before and after of the snipped that has changed and skip showing the results

[jimmyca15]

How about something like :
"Importing configurations with values as escaped json strings and the content type "application/json" is no longer supported. Please specify value as a json object."

I would avoid the phrase "is no longer supported." It's not that we removed support but rather we changed the behavior. We now convert the values of properties in the settings to json strings.

I was thinking since the breaking change is in relation to importing configurations, we can show the before and after of the snipped that has changed and skip showing the results

Sounds reasonable. But, I would include the content type that goes with the associated input snippet

[MaryanneNjeri]

I would avoid the phrase "is no longer supported." It's not that we removed support but rather we changed the behavior. We now convert the values of properties in the settings to json strings.

Yes good point.

[MaryanneNjeri]

This has been updated thanks

[bbrandt]

Does specifying the feature flags as the feature management schema for import then get converted to the App Config service feature flag prefix (.appconfig.featureflag/) and content type for feature flags by the extension or the app config service? So that the imported feature flags appear in the Feature Manager view alongside feature flags manually created in the portal?