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

change Billing Email -> Email #92

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

tessus
Copy link
Contributor

@tessus tessus commented Oct 28, 2022

@stefan0xC
Copy link
Contributor

turns out there were also a few whitespace errors in the patch file, which my editor fixed.

if they were in the source files we may not be able to apply the patches. Have you tested this?

@tessus tessus force-pushed the fix/billing-email branch from 8206a54 to ff1db50 Compare October 29, 2022 00:05
@tessus
Copy link
Contributor Author

tessus commented Oct 29, 2022

I now used make generate-patch

@stefan0xC
Copy link
Contributor

Can you rename the file to patches/v2022.10.0.patch so your change between the two versions is easier to see? I don't think it's necessary to create a new patch file for this.

@tessus tessus force-pushed the fix/billing-email branch from ff1db50 to da840ee Compare October 29, 2022 00:37
Copy link
Contributor

@stefan0xC stefan0xC left a comment

Choose a reason for hiding this comment

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

I don't think there is a benefit of keeping the organization email displayed as Billing Email for our intended target audience ("individuals, families, and smaller organizations") since Vaultwarden does not implement the subscription plans or billing system anyway and the suggested change of just displaying Email is minimal and maintaining it should be easy.

@tessus
Copy link
Contributor Author

tessus commented Oct 29, 2022

Thanks for your comment. I am more than happy to take care of this in the future. Should this ever generate any issues, just ping me and tell me to fix this. ;-)

@BlackDex
Copy link
Collaborator

BlackDex commented Nov 8, 2022

Can you rename the file to patches/v2022.10.0.patch so your change between the two versions is easier to see? I don't think it's necessary to create a new patch file for this.

Actually, it is better to have the later version here, so v2022.10.2, becuase we want to have some form of change chain. That this specific fix was not in the 2022.10.0 patch before.

We even create a v2022.10.2a for example when we make some other fixes, so that we at least know if someone is using the wrong vault.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

I think this is fine, though not a must for me personally to have this changed.

@BlackDex
Copy link
Collaborator

BlackDex commented Nov 8, 2022

Not sure if this will end-up into the current version. I might still want this to have the file changed to v2022.10.2 though.
But i think v2022.11.0 will be released any time soon.

@tessus
Copy link
Contributor Author

tessus commented Nov 8, 2022

Ok, but let me ask you this. Since v2022.10.2 is already released, should I not rename the file to v2022.10.2a? For me it doesn't matter, I just want to understand how and why.

@BlackDex Please let me know, I'll rename it right away:

  • v2022.10.2.patch
  • v2022.10.2a.patch

Edit: updated the version numbers.

@BlackDex
Copy link
Collaborator

BlackDex commented Nov 8, 2022

Well, v2022.11.x is not released yet, so none of those :).
In theory it should be named v2022.10.2a, since indeed we already have a v2022.10.2 release.
Not sure how @dani-garcia looks at this, but there isn't a .10.2 patch, only a .10.0.

In theory that could mean a patch file named v2022.10.2 and a new tagged release called v2022.10.2a.
However, to be consistent, the patch file should then be called v2022.10.2a also. So, in the end i would go for that, that is more clear i think.

@tessus
Copy link
Contributor Author

tessus commented Nov 8, 2022

Oops, I updated the version numbers in my previous comment, but I don't think that gh sends another email for an edit.

Btw, you said that v2022.11.0 might be released soon. This would require at least dani-garcia/vaultwarden#2893 and dani-garcia/vaultwarden#2899 to be merged. However, right now there has not even been a 1.27.0 vw release (with the group support and other changes - current main).
I am trying to understand the release schedule and versioning. e.g. let's say v2022.11.0 comes out, but it will only work with the above 2 fixes in the server. A package maintainer would add this as a requirement, but can't know the version number that will include these 2 PRs in advance. Thus a roadmap or an idea how the versioning works could help a lot.

@tessus
Copy link
Contributor Author

tessus commented Nov 8, 2022

I can squash the 2 commits.

Some projects do not want a force push in a PR, since it will destroy the comment history. Please advise.

@BlackDex
Copy link
Collaborator

BlackDex commented Nov 8, 2022

I can squash the 2 commits.

Some projects do not want a force push in a PR, since it will destroy the comment history. Please advise.

I think this is fine. It's not that there are 30 commits ;)

@dani-garcia dani-garcia merged commit 17dcf95 into dani-garcia:master Nov 9, 2022
@dani-garcia
Copy link
Owner

Well, v2022.11.x is not released yet, so none of those :). In theory it should be named v2022.10.2a, since indeed we already have a v2022.10.2 release. Not sure how @dani-garcia looks at this, but there isn't a .10.2 patch, only a .10.0.

In theory that could mean a patch file named v2022.10.2 and a new tagged release called v2022.10.2a. However, to be consistent, the patch file should then be called v2022.10.2a also. So, in the end i would go for that, that is more clear i think.

The names of the patches are mostly informational, really, with our current build system it tries to match the vault version you're selecting against the patches and uses the latest one if there's no match, but in the docker builds we are using the commit hash, so it will always just select the latest one.

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.

5 participants