Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 publishing to the dotnetbuilds storage account alongside dotnetcli #7877
Add publishing to the dotnetbuilds storage account alongside dotnetcli #7877
Changes from 1 commit
bf41bf0
6ba4be3
1dfe763
7fa8f1b
d073ef7
060c3d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the feed overrides are present, we should write a test that ensures that an asset is only pushed to a target endpoint once. In the new configuration here, there are multiple target feed configs for an installer blob, for instance. That might cause a bad interaction when the target is overridden to be a specific feed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... troubling. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just let the base classes handle this if they want? Rather than forcing everyone to call base(log)?
This abstract class isn't really adding any functionality that an interface with the single "PublishAssetAsync" doesn't provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the implementations needed it, and calling base(log) isn't really difficult. An interface would play nicer with DI if we do it in the future, so I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be removed (maybe just stick an NYI in the case). We should not be publishing using Sleet any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FeedType.AzureStorageFeed == publishing to dotnetcli, I don't think we can remove that. Unless the code does very different things than from what I interpreted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm..I thought that was "AzureStorageContainer".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AzureStorageContainer is the new one I made, that uses a SAS token to publish, AzureStorageFeed is the old thing that looks and smells like sleet, but just strips the index.json off the end and publishes blobs like normal. It uses the account key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Can you do a quick refactor on the AzureStorageFeed enum name to be more indicative of its purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can... return from a constructor? Is that... wise? This feels like it should throw, since nothing later has anything like "did the complete normally" which is a weird thing to have to ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, you can return. This is this way because it was that way where I moved it from, but it should 100% be a throw, I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you hoist this constant out (and other similar constants). We've had to update these a few times in the past and its never easy to actually find which ones to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to only happen in "disposing==true". If it's fale, the _httpClient might already be GC'd. (antoher reason maybe having the base class isn't a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should wrap this
{}
and scope appropriately. I seem to remember changing this specifically a while back for either performance or correctness reasons. IIRC, PackageArchiveReader took an exclusive lock on the file, and depending on disposal time (since in this configuration the scope goes to the end of the method), we got occasional lock issues.Anyways, scoping down to the minimal required scope is also good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this option can be just removed at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its set to true in all cases I can see, so I don't see how we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean remove the conditional altogether. I don't see us ever passing
false
for this.