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

Create empty blob if not exist. #35

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

LiquoriChris
Copy link
Contributor

No description provided.

@LiquoriChris
Copy link
Contributor Author

@NowinskiK sorry for the quick pull request. Found a bug in the incremental deployment. Sorry for the inconvenience.

@NowinskiK
Copy link
Member

Related to #34

@NowinskiK
Copy link
Member

@LiquoriChris, can you make sure first you have the latest version of the code from main branch?
Also, before (or now after) fixing the bug, could you cover the case with an appropriate unit test?

@LiquoriChris
Copy link
Contributor Author

@NowinskiK main now synced, tests updated, and updated changelog.

@LiquoriChris LiquoriChris reopened this May 1, 2024
@NowinskiK
Copy link
Member

Hi @LiquoriChris, could you please fix these small comments I left?

@LiquoriChris
Copy link
Contributor Author

Hey @NowinskiK, can you please re-check the code? I have updated when you mentioned in your first comment. Please let me know if I have missed anything.

@LiquoriChris
Copy link
Contributor Author

Hey @NowinskiK, can you please re-check the code? I have updated when you mentioned in your first comment. Please let me know if I have missed anything.

Hey @NowinskiK, sorry to bother. Can you please see comment above?

@NowinskiK
Copy link
Member

There are some unexpected changes you introduced, like this one:
image

@LiquoriChris
Copy link
Contributor Author

Apologies. I removed the quotes in the original pr, but added them back in. Sorry about that.

@NowinskiK
Copy link
Member

That's all right, but that was one of the reasons why it was with you.
There is another comment above - please scroll and check. Optionally you can check "Files changed" section (tab).

@LiquoriChris
Copy link
Contributor Author

LiquoriChris commented Jul 17, 2024

I do not think I can see comments in the files changed. It does not show any comments in the files changed tab.

image

private/DeploymentState.class.ps1 Outdated Show resolved Hide resolved
private/DeploymentState.class.ps1 Outdated Show resolved Hide resolved
}
Catch {
throw $_
}
Copy link
Member

Choose a reason for hiding this comment

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

Why TRY in TRY? Can't be more elegant pls?

Copy link
Contributor Author

@LiquoriChris LiquoriChris Jul 24, 2024

Choose a reason for hiding this comment

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

Removed nested try/catch blocks.

@NowinskiK
Copy link
Member

ah... thanks! I should click "Request changes" then all my comments are moved here (see above).

@LiquoriChris
Copy link
Contributor Author

Hello @NowinskiK I have cleaned up the code (Thanks for that suggestion). I also updated the tests to make sure they pass.

@LiquoriChris LiquoriChris requested a review from NowinskiK July 24, 2024 17:46
}
Catch {
throw $_.Exception
}
Copy link
Member

Choose a reason for hiding this comment

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

Why catch an error and throw an exception? Why just do not remove the TRY/CATCH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NowinskiK Please see updated code.

catch {
Write-Verbose $_.Exception
Catch {
throw $_.Exception
}
Copy link
Member

Choose a reason for hiding this comment

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

The same here (as above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NowinskiK Please see updated code.

}
catch {
Catch {
throw $_.Exception
}
Copy link
Member

Choose a reason for hiding this comment

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

The same here (as above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NowinskiK Please see updated code.

Copy link
Member

@NowinskiK NowinskiK left a comment

Choose a reason for hiding this comment

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

I left few comments around TRY/CATCH blocks.

@LiquoriChris
Copy link
Contributor Author

Hello @NowinskiK,

I know it has been a while. Can you please see if this is still ok? I've updated the code per comments.

@NowinskiK
Copy link
Member

NowinskiK commented Nov 27, 2024

But... there are still TRY/CATCH blocks. Example: DeploymentState.class.ps1 file. Why is that?
You can check this code for reference:
https://github.com/Azure-Player/azure.datafactory.tools/blob/master/private/DeploymentState.class.ps1
It's ADFTools with incremental deployment with storage implemented.

@LiquoriChris
Copy link
Contributor Author

I added try/catch just in case of failure... But I can remove entirely.

@NowinskiK
Copy link
Member

My point is that it does nothing. You catch potential error and rethrow it. I can't see the logic.

@LiquoriChris
Copy link
Contributor Author

I see your point. Removed.

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.

2 participants