Skip to content

AzureFunctionApp@2: Better alias for Service Connection #18229

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

Closed
wants to merge 11 commits into from

Conversation

nis-spiir
Copy link

@nis-spiir nis-spiir commented May 3, 2023

Task name: AzureFunctionApp@2

Description: "Azure Subscription" is a terrible variable name for something that is not an Azure Subscription.

Documentation changes required: Y (adds alias, so auto-generated docs needs to be updated)

Added unit tests: N

Attached related issue: Y https://github.com/MicrosoftDocs/azure-devops-docs/issues/13281

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

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.

Hello,

I'm the owner of the Azure Functions deployment tasks.
Was there something that prompted this change?

This syntax is common to a lot of other tasks and changing the parameter in such a manner has a level of risk in relation to breaking existing pipelines, samples, templates etc?

For these reasons I would prefer not to make such a change.

Thanks,

Finbar

@nis-spiir
Copy link
Author

nis-spiir commented May 15, 2023

/Hey @FinVamp1

What prompted this PR is https://github.com/MicrosoftDocs/azure-devops-docs/issues/13281

I'm seeing a lot of developers being confused about "service connections" especially when connecting to Azure where "azureSubscription" should not be the name of an Azure Subscription 🙃

It seems like "azureSubscription" is a legacy name? I see that even in this task you directly map the public azureSubscription input to connectedServiceName so clearly "azureSubscription" is not a logical name for that parameter (see https://github.com/microsoft/azure-pipelines-tasks/blob/master/Tasks/AzureFunctionAppV2/taskparameters.ts#LL21C17-L21C17 )

This syntax is common to a lot of other tasks

I just checked (find Tasks/ -iname task.json | xargs cat | jq '.inputs | .[] | select(.name == "azureSubscription", .name == "connectedServiceName", .name == "connectedServiceNameARM") | .name') and these are the stats for variable name:

There are 5 tasks with azureSubscription being the primary variable name (i.e. this task one of them)

There are 7 tasks with connectedServiceName being the primary variable name

There are 9 tasks with connectedServiceNameARM being the primary variable name

changing the parameter in such a manner has a level of risk in relation to breaking existing pipelines, samples, templates etc?

I can't find anywhere that documents this will break stuff? Adding a new alias should be backwards compatible, but the script generating docs will now list (a better) variable name.
I haven't changed the version attribute - but I guess this would just be a minor update?

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@nis-spiir
Copy link
Author

Is this PR making it fail? Or is it a broken/flaky test? (I can't see the ADO output, just Bash exited with code '1'.)

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

@nis-spiir Please rebuild the task and run the test cases again and check if any of the test cases failing in the repo.

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

@nis-spiir Can you please bump the version of the task. I could see the current version of the task is same as the one in PR.

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@FinVamp1
Copy link
Contributor

azp /run

@FinVamp1
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@nis-spiir
Copy link
Author

Do I need to do anything more to get this merged?
The error [1] seems to indicate that build fails due to a pipeline problem (and not this PR)?

[1] Cannot access to https://feeds.dev.azure.com/mseng/PipelineTools/_apis/packaging/Feeds/DistributedTasks/packages?api-version=7.0&includeAllVersions=true due to error Error: Failed request: (401)

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@v-nagarajku
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@FinVamp1 FinVamp1 requested review from mmrazik and manolerazvan June 20, 2023 14:31
@v-nagarajku
Copy link
Contributor

@nis-spiir I have cloned the same file changes from this PR and created a new PR #18522 from my local system and checks are getting succeeded. Feel free to review PR changes.

cc: @FinVamp1

@v-nagarajku
Copy link
Contributor

Before:
image

After:
image

Here the only change is from Azure subscription to Azure Resource Manager connection is it right?

cc: @nis-spiir @FinVamp1

@v-nagarajku
Copy link
Contributor

Hi @nis-spiir I created this PR #18522 to help in fixing this current PR #18229 . Can you please update this PR to take further actions.

@v-nagarajku
Copy link
Contributor

Closed this PR as clone of this has been merged. #18522

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.

3 participants