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

Feature - add set data factory trigger state #46

Conversation

stijnmoreels
Copy link
Member

Closes #18

@mbraekman
Copy link
Contributor

Would it be possible to split the current Set-AzDataFactoryTriggerState into 2 separate commands:

  • Start-AzDataFactoryTrigger
  • Stop-AzDataFactoryTrigger

This makes it more user-friendly.

@stijnmoreels stijnmoreels marked this pull request as ready for review June 15, 2020 12:20
@stijnmoreels stijnmoreels requested a review from mbraekman as a code owner June 15, 2020 12:20
Copy link
Contributor

@mbraekman mbraekman left a comment

Choose a reason for hiding this comment

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

some minor remarks.
Also don't forget to update the *.psd1-file (FunctionsToExport-property) with the adjusted command-names, as suggested in the comments.

docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, just added a few suggestions.

Would it be possible to split the current Set-AzDataFactoryTriggerState into 2 separate commands:

  • Start-AzDataFactoryTrigger
  • Stop-AzDataFactoryTrigger

This makes it more user-friendly.

Shouldn't this be more Enable & Disable @mbraekman?

docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/features/powershell/datafactory.md Outdated Show resolved Hide resolved
docs/preview/index.md Outdated Show resolved Hide resolved
@mbraekman
Copy link
Contributor

@tomkerkhove : I was asking myself the same question yesterday, regarding the enable/disable vs start/stop. The reason I eventually didn't make this remark was because the underlying Az-command uses Start/Stop as well, this of course doesn't mean we cannot use enable/disable instead.

Guess we'll be using more of enable/disable throughout other scripts as well, so let's make it consistent and go for enable/disable throughout the repo.

@stijnmoreels:
can you update the commands to reflect this, please?

@tomkerkhove
Copy link
Contributor

Guess we'll be using more of enable/disable throughout other scripts as well, so let's make it consistent and go for enable/disable throughout the repo.

The built-in ones have always confused me because it makes me feel like I'm triggering the pipelines instead of enabling/disabling 😅

@stijnmoreels
Copy link
Member Author

@stijnmoreels:
can you update the commands to reflect this, please?

Yes, will do.

@stijnmoreels
Copy link
Member Author

@stijnmoreels:
can you update the commands to reflect this, please?

Yes, will do.

Done!

Copy link
Contributor

@mbraekman mbraekman left a comment

Choose a reason for hiding this comment

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

Some final remainders of the start/stop-rename to enable/disable.
But other than that, looks good to me!

Approving this, so you can go ahead and merge once those last items are ticked off.

@stijnmoreels
Copy link
Member Author

Some final remainders of the start/stop-rename to enable/disable.
But other than that, looks good to me!

Aha! Thanks, with all this renaming was slipped through 😅 .

@stijnmoreels stijnmoreels merged commit fe45378 into arcus-azure:master Jun 18, 2020
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.

Provide scripts to enable/disable a DataFactory v2 trigger
3 participants