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

[Identity] Changelog entry for the recent DeviceCodeCredential change #11475

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

sadasant
Copy link
Contributor

This is an experiment. It's a separate PR with the changelog entry for this previous PR: #11355

I'm trying to separate the code changes from the changes related to documentation etc. I'm wondering if it is easier to review this this way or if I should always add the changelog entry as part of the PR with the code changes.

@sadasant sadasant self-assigned this Sep 24, 2020
@ghost ghost added the Azure.Identity label Sep 24, 2020
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Sep 24, 2020

I'm trying to separate the code changes from the changes related to documentation etc. I'm wondering if it is easier to review this this way or if I should always add the changelog entry as part of the PR with the code changes.

Generally, I recommend adding changelogs as part of the PR that contains the code changes simply to avoid the case where we forget to add them and get release time stress in collecting all the changelog entries. Having both in the same PR avoids the context switch that one would have when reviewing the second PR if it comes a while after the first one.

If it helps, you can consider review comments on the changelog in the first PR that has code changes as non blocking and always follow up in a second PR. Hopefully, as time goes, the first attempts would improve and not need followups

@@ -1,7 +1,8 @@
# Release History

## 1.2.0-beta.2 (Unreleased)
- Reverted change in 1.2.0-beta.1 which moved @rollup/plugin-json from devDependencies to dependencies
- Reverted change in 1.2.0-beta.1 which moved @rollup/plugin-json from devDependencies to dependencies.
- `DefaultAzureCredential` now by default shows the Device Code message on the console. This can still be overwritten with a custom behavior by specifying a function as the third parameter, `userPromptCallback`.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the default be to not prompt, especially since this shipped? This seems like a breaking, blocking behavior change as it waits for user input. In .NET, we have a boolean parameter to opt into interactive prompts (or go full-on pick-and-choose with an options class).

Please bear in mind, I'm only seeing this description, though. Perhaps I'm misinterpreting it having not seen the code review, but it really does seem to indicate you'll block on user input by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time, Heath!

The changes in my PR only add a logger by default. They don't affect the blocking behavior.

@sadasant
Copy link
Contributor Author

sadasant commented Sep 28, 2020

I'd like to state that I believe the number of feedback entries in this PR justifies separating it from the source code changes! Here we can focus on the changelogs, without conflating the implementation. The Implementation feedback happened to be very concise. These conversations, although wonderful and absolutely necessary, are in my perspective an entirely separate effort than the implementation.

@sadasant sadasant force-pushed the identity/11355-changelog branch from 42a8b92 to 1feae28 Compare September 29, 2020 20:45
@sadasant sadasant requested a review from heaths September 29, 2020 20:51
@sadasant sadasant merged commit 0a9a225 into Azure:master Sep 30, 2020
@sadasant sadasant deleted the identity/11355-changelog branch September 30, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants