Skip to content

Fix invalid SAS token from New-AzServiceBusAuthorizationRuleSASToken and New-AzEventHubAuthorizationRuleSASToken#14535

Merged
wyunchi-ms merged 9 commits intoAzure:masterfrom
fsackur:12975-invalid-token
Mar 16, 2021
Merged

Fix invalid SAS token from New-AzServiceBusAuthorizationRuleSASToken and New-AzEventHubAuthorizationRuleSASToken#14535
wyunchi-ms merged 9 commits intoAzure:masterfrom
fsackur:12975-invalid-token

Conversation

@fsackur
Copy link
Copy Markdown
Contributor

@fsackur fsackur commented Mar 14, 2021 •
edited
Loading

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@ghost ghost added the customer-reported label Mar 14, 2021
@ghost
Copy link
Copy Markdown

ghost commented Mar 14, 2021

Thank you for your contribution fsackur! We will review the pull request and get back to you soon.

@fsackur fsackur force-pushed the 12975-invalid-token branch from 87f4da3 to 6926299 Compare March 14, 2021 01:03
@fsackur
Copy link
Copy Markdown
Contributor Author

fsackur commented Mar 14, 2021 •
edited
Loading

I am not a C# dev and I don't offer enough of my time to get up to speed on the build steps.

In the case of 12975, if the code compiles, it is impossible for me to make the codebase worse, because the cmdlet is utterly broken. I'm pretty sure that no-one has ever successfully used it. AFAICS, it has literally never been tested.

In the case of 14534, this had previously been fixed in the case where StartTime is not supplied; therefore the last three commits might need to be dropped from the PR if my code sucks.

To save the reviewer time, you may do whatever is expedient with the history; if you wish to drop commits, if you wish to rewrite the history in my branch, go ahead. This branch is just me trying to get some activity on a very frustrating and long-standing bug.

wyunchi-ms
wyunchi-ms previously approved these changes Mar 15, 2021
View reviewed changes
@v-Ajnava
Copy link
Copy Markdown

v-Ajnava commented Mar 15, 2021

/azp run

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Mar 15, 2021

Azure Pipelines successfully started running 3 pipeline(s).

@v-Ajnava
Copy link
Copy Markdown

v-Ajnava commented Mar 15, 2021

LGTM waiting for CIs to be green

@fsackur
Copy link
Copy Markdown
Contributor Author

fsackur commented Mar 15, 2021

Looking at the build errors now, will update

@fsackur
Copy link
Copy Markdown
Contributor Author

fsackur commented Mar 15, 2021

/azp run

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Mar 15, 2021

Commenter does not have sufficient privileges for PR 14535 in repo Azure/azure-powershell

@fsackur
Copy link
Copy Markdown
Contributor Author

fsackur commented Mar 15, 2021

Hi @v-Ajnava , I think I've fixed the syntax errors, it does at least build on my machine. Could you rerun the pipeline?

@v-Ajnava
Copy link
Copy Markdown

v-Ajnava commented Mar 16, 2021

/azp run

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Mar 16, 2021

Azure Pipelines successfully started running 3 pipeline(s).

@wyunchi-ms wyunchi-ms merged commit 93beeb8 into Azure:master Mar 16, 2021
@wyunchi-ms
Copy link
Copy Markdown
Contributor

wyunchi-ms commented Mar 16, 2021

Merged. Thanks a lot for your contribution! @fsackur

@v-Ajnava
Copy link
Copy Markdown

v-Ajnava commented Mar 16, 2021

Thanks @fsackur !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyunchi-ms wyunchi-ms wyunchi-ms approved these changes

Assignees

@fsackur fsackur

@v-Ajnava v-Ajnava

@wyunchi-ms wyunchi-ms

Labels

customer-reported

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fsackur @v-Ajnava @wyunchi-ms