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

Add application insights connection string support #19855

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Greybird
Copy link

@Greybird Greybird commented May 10, 2024

Task names:

  • AzureFunctionAppContainerV1
  • AzureFunctionAppV1
  • AzureFunctionAppV2
  • AzureRmWebAppDeploymentV3
  • AzureRmWebAppDeploymentV4
  • AzureWebAppContainerV1
  • AzureWebAppV1

Description:
The PR adds support for adding release annotations when the now recommended APPLICATIONINSIGHTS_CONNECTION_STRING setting is used on Azure, rather than the APPINSIGHTS_INSTRUMENTATIONKEY, for all tasks supporting this feature.

Documentation changes required:
I don't think so.

Added unit tests:
No.
I did not find tests for release annotation based on InstrumentationKey.
I had a hard time figuring how to add unit tests for the connection string use case, and tests seem to come from another zone of the code.
Happy to get guidance on the matter however.

Attached related issue:
#18796 (closed due to being stale, probably needs reopening)

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected (tested on our organization by publishing a preview task as explained in documentation)

Copy link
Contributor

@FinVamp1 FinVamp1 left a comment

Choose a reason for hiding this comment

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

Can you update the Task.json as mentioned?

@Greybird
Copy link
Author

Can you update the Task.json as mentioned?

Sure, I just did, and updated issue template too. Feel free to point any mistake I would have made in the process of course!

Copy link
Contributor

@FinVamp1 FinVamp1 left a comment

Choose a reason for hiding this comment

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

Looks good to me from the Function side of things. Thanks for the contribution.

Arnaud TAMAILLON added 2 commits May 11, 2024 10:50
…s-connection-string

# Conflicts:
#	Tasks/AzureRmWebAppDeploymentV3/task.json
#	Tasks/AzureRmWebAppDeploymentV3/task.loc.json
#	Tasks/AzureRmWebAppDeploymentV4/task.json
#	Tasks/AzureRmWebAppDeploymentV4/task.loc.json
#	_generated/AzureRmWebAppDeploymentV3.versionmap.txt
#	_generated/AzureRmWebAppDeploymentV3/task.json
#	_generated/AzureRmWebAppDeploymentV3/task.loc.json
#	_generated/AzureRmWebAppDeploymentV3_Node20/task.json
#	_generated/AzureRmWebAppDeploymentV3_Node20/task.loc.json
#	_generated/AzureRmWebAppDeploymentV4.versionmap.txt
#	_generated/AzureRmWebAppDeploymentV4/task.json
#	_generated/AzureRmWebAppDeploymentV4/task.loc.json
#	_generated/AzureRmWebAppDeploymentV4_Node20/task.json
#	_generated/AzureRmWebAppDeploymentV4_Node20/task.loc.json
@Greybird
Copy link
Author

hi @jvano, @dannysongg, @manolerazvan, @patelchandni,
Sorry to ping you directly ; do not hesitate to redirect me to proper instructions if I missed anything.

Could you guide me on how to have this pr be reviewed and integrated ?
I could take quite a long time to have all reviews, and as we have to integrate sprint number into the PR, I might have to merge/update quite often as the number of people involved is large.

Copy link

@jvano jvano left a comment

Choose a reason for hiding this comment

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

:shipit:

@Greybird
Copy link
Author

Thanks @jvano, @manolerazvan !

@dannysongg, @patelchandni, could you please take a look ?

@FinVamp1
Copy link
Contributor

Thanks @jvano, @manolerazvan !

@dannysongg, @patelchandni, could you please take a look ?

Hello @Greybird I think in this instance Joaquin and Danny are part of the Web Apps task team and Chandni and I are part of the Functions team so just one approval is probably needed from each team.

@Greybird
Copy link
Author

@FinVamp1, you are right, sorry,
I have been confused by the "2 pending reviewers" mention, while the changes are actually approved. Thanks for pointing this!

I suppose there is nothing left for me to do currently, as checks now have to run, which I believe depends on some validation on Microsoft side ?
Do not hesitate to point to needed actions if I am wrong.

Thanks again for your guidance with the process !

@Greybird
Copy link
Author

Greybird commented Jun 3, 2024

Hello,

I updated sprint to 241.
I don't know how I can help further to have this PR merged.

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.

None yet

4 participants