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 braces to '-and' in if statements, for the correct function handling #15458

Merged
merged 13 commits into from
Mar 10, 2022

Conversation

kuleshovilya
Copy link
Contributor

@kuleshovilya kuleshovilya commented Oct 28, 2021

Description: Add braces to '-and' in if statements, for the correct function handling
Attached related issue: DTS 1887373
Documentation changes required: N
Added unit tests: N

Checklist:

  • Checked that applied changes work as expected

@kuleshovilya kuleshovilya requested a review from a team October 28, 2021 15:02
@anatolybolshakov
Copy link
Contributor

Should we bump version for some tasks - to consume these changes?

@kuleshovilya kuleshovilya changed the title Users/kuleshovilya/fix incorrect and param Add braces to '-and' in if statements, for the correct function handling Oct 28, 2021
Copy link
Contributor

@v-jkarri v-jkarri 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 please undo version changes for AzureCloudPowerShellDeployment and
SqlAzureDacpacDeployment tasks. As the method Disconnect-AzureAndClearContext in Tasks/Common/VstsAzureHelpers_/Utility.ps1 is used only in AzureFileCopy and AzurePowerShell tasks.

@marcelovital
Copy link

Does this do the same as #15402 ?

@marcelovital
Copy link

Can you please finish this PR? It's stopped for a long time, and also seems similar to my #15402

@DiederickA
Copy link

@chshrikh @nadesu @microsoft/akvelon-build-task-team: can you review this PR so it will continue?

@nadesu nadesu requested review from PhilipsonJoseph and v-saikumart and removed request for chshrikh and nadesu February 4, 2022 04:27
@nadesu
Copy link
Contributor

nadesu commented Feb 4, 2022

@PhilipsonJoseph @v-saikumart @v-ibshaik - Pls review the PR

@jberezanski
Copy link

Could this please be merged? The deluge of these warnings in our pipelines makes it much more difficult to recognize actual problems.

@v-lomer
Copy link

v-lomer commented Mar 3, 2022

I agree that this issue is only impacting all versions (1 thru 5) of AzureFileCopy and all versions (2 thru 5) of AzurePowerShell through their usage of function Disconnect-AzureAndClearContext (Tasks\Common\VstsAzureHelpers_\Utility.ps1).

The two impacted functions (function Disconnect-UsingAzModule and function Disconnect-UsingARMModule) are only used by function Disconnect-AzureAndClearContext (Tasks\Common\VstsAzureHelpers_\Utility.ps1)

  • Please undo version changes for AzureCloudPowerShellDeployment and SqlAzureDacpacDeployment tasks
  • #15402 does correct the -and issue with the two functions in question. While including more style-cop formatting issues and removes the usage of Aliases
  • Separate issue should be opened to focus on 15402's style-cop formatting issues and possible a 2nd issue for removing Aliases (these would focus on the whole codebase in team defined units)

@v-lomer
Copy link

v-lomer commented Mar 3, 2022

@PhilipsonJoseph This PR appears to have lost attention, is there anyone this can be escalated to for completion?

@v-lomer
Copy link

v-lomer commented Mar 5, 2022

@v-ibshaik This PR appears to have lost attention, is there anyone this can be escalated to for completion?

@DiederickA
Copy link

Could this be given some attention?
@kuleshovilya @v-ibshaik @PhilipsonJoseph

@kuleshovilya
Copy link
Contributor Author

@DiederickA As @v-lomer pointed out there's already a similar PR that fixes exact same issue, I suggest just closing this one.

@DiederickA
Copy link

I do not mind which one is resolved... That similar PR does not have a better track record.

@v-lomer
Copy link

v-lomer commented Mar 8, 2022

This PR follows One Definition Rule (ODR) so I would suggest finishing this PR over the other linked PR

@kuleshovilya
Copy link
Contributor Author

I'm not working on a related project anymore and not in the context of the changes currently, so I think either closing it or asking somebody whos more in the flow than me to finish it. Sorry for the inconvinience.

@DaniilShmelev
Copy link
Contributor

@rvairavelu Judging by the codeowners file, this is your task. Could you please take a look?

@PhilipsonJoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@PhilipsonJoseph
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@PhilipsonJoseph PhilipsonJoseph merged commit dbf9d8c into master Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants