Add support for publishing to Azure storage #413
Conversation
chuan
changed the title
|
@smira Any update on this? I would love to see this feature integrated. |
|
I attempted to use this PR and found that not only there are some minor conflicts that popped up during a rebase, but also the Azure module used here has changed the API in a completely incompatible way and I don’t have enough knowledge on either Azure API or Go to be able to port it. @chuan, could you please forward-port it? Thanks in advance. |
|
@andrewshadura I can give it a try if there are still interests. |
Hi 🙂 I would certainly be great if this could be updated. We’re currently using aptly with a minio proxy, so one fewer proxy would be helpful 😄 |
|
@andrewshadura I have rewritten the Azure publish module based on latest blob storage sdk. Please give it a try. I am no longer maintaining any repo using aptly. I only verified with unit tests and some simple publishing commands. |
|
@lbolla, could you please have a look at this? |
|
@andrewshadura It looks good to me at first sight, but I don't have the ability to test it properly in Azure. Do you? My only concern is to include many "Azure" dependencies in aptly, which may not be needed by most users. But I can't think of an easy way to support Azure while keeping those dependencies optional. All in all, it is a nice patch, and pretty self-contained, so I am inclined to merge it. |
|
btw, it'll have to be rebased to master. @andrewshadura or @chuan could you? |
|
I think I can give it a go tomorrow. I have an Azure account I can probably test this with. |
|
I’m rebasing this now. |
|
Thanks @andrewshadura and @lbolla. I have rebased my PR to latest master. |
|
@andrewshadura I am not very comfortable that you created a new PR in your name before I abandoned my effort to get this PR accepted. I normally did not care that much. But after 6 years, I guess I am a little attached to this PR. That said, if there is any merits in favor of your PR, I would like to hear it out nevertheless. |
|
Well, I did not remove your name, I didn't take credit for your work, so I'm not sure why you take it personally. To be honest, I never considered that a clean-up of others' contributions could be received negatively.
My version of your PR is better because I removed lots of uninteresting intermediate steps like "more golint fixes", vendoring/unvendoring, old API etc; I also regrouped commits and expanded commit messages. When others do this to my pull requests, I'm usually happy my work gets improved.
…On Mon, 20 Dec 2021, at 19:34, Chuan Liu wrote:
@andrewshadura <https://github.com/andrewshadura> I am not very comfortable that you created a new PR in your name before I abandoned my effort to get this PR accepted. I normally did not care that much. But after 6 years, I guess I am a little attached to this PR.
That said, if there is any merits in favor of your PR, I would like to hear it out nevertheless.
—
Reply to this email directly, view it on GitHub <#413 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLQBKCCVQMFGLCSLLPCJDUR5ZNHANCNFSM4CGPOR3A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
Cheers,
Andrej
|
|
And another thing. From your last message it did not seem you were too interested in finalising this PR, whereas for me it's something that would allow me to greatly simplify the existing infrastructure we have. I guess I may have misunderstood your intentions, but since I had time and interest in doing this, I thought I'd give it a go.
…On Mon, 20 Dec 2021, at 19:34, Chuan Liu wrote:
@andrewshadura <https://github.com/andrewshadura> I am not very comfortable that you created a new PR in your name before I abandoned my effort to get this PR accepted. I normally did not care that much. But after 6 years, I guess I am a little attached to this PR.
That said, if there is any merits in favor of your PR, I would like to hear it out nevertheless.
—
Reply to this email directly, view it on GitHub <#413 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLQBKCCVQMFGLCSLLPCJDUR5ZNHANCNFSM4CGPOR3A>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
Cheers,
Andrej
|
|
You see, *someone* had to rebase this. Be it you, me, or one of the maintainers. In fact, it's not uncommon that maintainers rebase patches, change things, reword commit messages etc before merging. Would you be less unhappy if @lbolla did this work? And if yes, what difference does it make?
|
|
I never abandoned this PR, and revisited it time and again in the past 6 years as shown in the history above. I am sorry my words gave your the wrong impression that I was no longer working on it. I am no longer maintaining (debian) repos using aptly, however, I am still interested in getting my change accepted by an open source project I used in the past and might use again in the future. I would also love to see it benefit aptly community and users. If you want to clean up the commit history in this PR, you can let me know. I am more than happy to do that as I forward-ported the change at your request. We can also ask for a squash & merge to mainline as it is a common practice for github projects, so there will be no messy history at all in mainline except one commit. If you want to contribute to this PR, you can open a PR against my branch and I would be more than happy to accept your change. If you want to independently contribute to the mainline, you can wait until this PR is merged to submit a new PR to improve the mainline. I appreciate your effort in promoting this change and testing it out voluntarily, and would like to give your credit and my thanks for that. But it is not ok to do what you were doing though. If your PR get merged, for anyone looking at the mainline commit history, it will appear to them you initiated the change and "borrowed" some code I made in an old PR. This is a distortion of effort and credit. If you PR get squash merged, there could be github technical issues (isaacs/github#1303) in displaying authorship properly. The maintainer also should not change my code during the commit to mainline. It is not only about authorship but bad software practice as it could introduce new bugs that were hard to trace in history. |
|
I disagree about this being not okay, and it’s pretty common in other projects I participate, as are pre-merge changes by the maintainers; I also don’t think it distorts authorship or effort, but let’s just agree to disagree on this. I’ve done the work which I think is useful and in my view the result is better than the current state of this PR; I think the best way forward would be for you to take my branch, reset the committer field if you prefer so and force-push it into this PR. To prevent confusion, I will close my PR shortly. |
I did not see any code change in your PR except squashing (or merging in your words) my commits. It is a github function to do squash and merge in a single button. I would leave it to the maintainer to decide if it is preferable to clean up the history in this branch pre-commit or just do a squash and merge when committing to mainline. Leaving the history in place has the benefit of record keeping and helps bug tracing in certain scenarios. But I am also happy to clean it up if it is aptly's preference to do normal merging and keeping history that way. @lbolla (I assume you are the current maintainer), could you comment on this? I saw you added a man page template change in your commit. As I said, you are welcome to submit PR to my branch or wait until this change is in to contribute it independently. |
Reviewing code, rebasing and squashing is work. I have split, reordered and recombined commits, squash button would have combined all that into a single commit.
Judging by the current history of the project, it prefers fast-forward merges with no merge commits and no squashing.
I don’t quite understand why you prefer to add extra steps instead of just taking commits from my branch, which is much easier and saves us all time. |
If it is my code, you should not change it without asking my permission first. Period. If I write a book with a messy draft, I may prefer the publisher to see it that way or I may prefer to clean it up. But it is up to me. You should not re-organize it and send it to publisher and claim credit (without my permission). It is that plain and simple. Also since it is my code (and work), won't I have the say how I want to clean it up, e.g. in one commit or in multiple commits?
As I said, I would leave it to the maintainer to decide. I don't think past dictates the future, and I am happy to accommodate the maintainers' preferences any way.
I can also do a clean cherry-pick of your change if that's what you would prefer. Do I get your permission to cherry-pick your change into my branch? |
|
@andrewshadura The unauthorized editorials to my commits aside, it is also rather not very practical (for me) to make changes to your branch if we uncover any new changes needed (to my code) before committing to the main branch. |
I cannot do a simple git cherry-pick from another fork--I think it is considered a different repo by git. You need to push your change to a branch in my fork. |
You can e.g. do this: |
Ok. Done. |
|
@lbolla, just a gentle reminder 🙂 |
|
Oh, I’ve just realised this: #413 (comment) — about adding more stuff to the commit messages — went unaddressed. |
Is this absolutely needed? I don't usually go back to change commit messages. Also the message you wanted to add seemed more like documentation to me. It probably should be added as a separate |
|
To me this seems like something useful in the commit message itself, not elsewhere. To someone digging through the history years later an empty commit message is much less useful than a message giving some useful bits of information. |
The commit message "add support for Azure storage as a publishing backend" already states what the commit is about. What you want to add is more detailed description about code and test behavior. And if we have proper documentation, why would someone need to dig through history? Even if they need to dig through, the code in the commit is quite self-contained and not too difficult to figure out in my opinion. Anyway, I will amend the commit since you insist. |
This adds a new configuration setting: AzurePublishEndpoints, similar to the existing S3PublishEndpoints and SwiftPublishEndpoints. For each endpoint, the following has to be defined: - accountName - accountKey - container - prefix Azure tests require the following environment variables to be set: - AZURE_STORAGE_ACCOUNT - AZURE_STORAGE_ACCESS_KEY With either of these not set, Azure-specific tests are skipped.
Co-authored-by: Andrej Shadura <andrew@shadura.me>
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
|
I have amended the commit message without the following sentence. Avoiding S3 + MinIO is irrelevant to the commit.
|
|
I guess our different expectations from commit messages come from different conventions of writing them. Most projects I participate in expect commit message for any non-trivial commit to have a detailed description of the change; the first line is a brief description of the change but it is not enough as it is basically treated as the Subject line of an email. |
|
Thanks for the update! |
|
@andrewshadura Are you going to approve the PR? Your reviewer status still shows "requested changes" to me. |
|
@chuan @andrewshadura Thank you both! |
|
@chuan @andrewshadura Hi all, We have noticed that this PR doesn't work with Go <1.13 The "azure" library seems to require at least go 1.13, although itself advertises version 1.15 https://github.com/Azure/azure-storage-blob-go/blob/master/go.mod What shall we do? We need support for go 1.11 at least, which is shipped with Debian buster. The plan at the moment is to remove Azure for old versions. Do you have a better idea? Maybe backport what's needed? EDIT: Maybe we could downgrade the "azure" library, which seems to have started to add constraints on Go versions in their version 0.8.0: https://github.com/Azure/azure-storage-blob-go/blob/v0.8.0/go.mod |
|
I don't think you need to support 1.11 for buster. Here's why:
1. Debian won't upload this version to buster anyway.
2. Go builds statically, so for your own packages you can use Go compiler from a later release.
3. Backports for buster ship 1.13, so you can build for buster using backports.
4. If Debian maintainers ever decide to upload this version to buster-backports-sloppy, they're going to use the 1.13 compiler from the backports.
|
|
@andrewshadura That's fair. @neolynx @randombenj What do you think? EDIT: actually, 1.13 fails too. 1.14 is required. See #1011 |
|
@lbolla If you need to build with go >= 1.13, I can try to downgrade the "azure-storage-blob-go" to 0.10.0 which should support go 1.13. |
|
@chuan we just set the minimum go version to 1.14, I think this should be fine |
|
@chuan , it is great feature, to publish to azure storage. |
|
@xumia Yes. I will take a look at it. |
|
Apologies to comment on this PR after it being closed, but would it be possible to have an example on how to publish to Azure now that it is available? The official tutorial on Aptly shows how to do it for Amazon S3, it would be great to have a similar example for Azure (I'm a beginner in both Azure and Aptly, so anything will help!) |
This is a PR to support Azure storage as a publishing backend as requested in #379.