search

lae / ansible-role-proxmox

IaC for Proxmox VE clusters.
MIT License
516 stars 146 forks source link

Update README.md #237

Closed willifehler closed 7 months ago

willifehler commented 7 months ago

The README.md should be updated to add compatibility about the lately merged Debian Bookworm support.

willifehler commented 7 months ago

Already done.

julianfoad commented 1 month ago

@willifehler Please check README.md second paragraph, "This role ... Debian Buster (10) and Bullseye (11)." and Role Variables section, has bullseye (not "bookworm" which I originally wrote here, a typo/thinko) as part of the default value of two values, and Developer Notes section, "Be sure to test any changes on both Debian 10 and 11 (update the Vagrantfile locally to use debian/buster64)"... Thanks.

lae commented 1 month ago

@julianfoad comments which reek of mental manipulation, using a method like Cunningham's Law, are not welcome here.

julianfoad commented 1 month ago

@lae I'm sorry my comment was unwelcome. It wasn't intended to be harsh or demanding or "manipulating". I only meant to point out something that seemed to have been overlooked or mistaken. Maybe I misunderstood what the previous poster meant. I quoted the specific texts that led to my doubt, so that it is clear why I questioned it, because I don't like comments that say something is broken without clearly stating what evidence led the commenter to say so.

lae commented 1 month ago

I don't like comments that say something is broken without clearly stating what evidence led the commenter to say so.

I'm in the same boat. However in this case, this is your first interaction (as far as I'm aware) with this project, and on a closed issue nonetheless, which immediately raises warning flags.

I quoted the specific texts that led to my doubt

The sort of effort you took would've been better off submitted as a patch/PR in my opinion, but in the context of necroing this issue and the following inaccurate statement:

and Role Variables section, has "bookworm" as part of the default value of two values

You don't specify that Bookworm isn't mentioned in those areas, and in fact say the opposite as if to indicate that it is. Thus the comment could be interpreted as being purposefully wrong and an engineering attempt to elicit a "wait, no, that's wrong and all of these should be fixed" reaction. As a side note, OP possibly closed this because "PVE 8.x" (which is Bookworm) was added in the README.

That said, clearly I was off about your intentions, sorry.

julianfoad commented 1 month ago

Sorry again... as it is (yes) my first interaction here I suppose I could have started with "thanks for making this project, and here's something I came across in my first experience, and here's why I thought it worth reporting, and in case it's not clear I mean these specific places...".

I almost chose to submit a PR instead, but was not totally sure these are the changes you would want (either in specifics, or at all if I misunderstood something big like it's already done on another branch).

In Role Variables section, that was my typo -- I wrote the wrong codename. I'm always getting confused between the three codenames beginning with "b". Sorry again, that just added to the difficulty here.

Thanks for your apology about my intentions; accepted.

lae commented 1 month ago

Yeah, those were just oversights so it's fine to add them. I'll accept a PR for it, or otherwise update the doc myself later.

julianfoad commented 1 month ago

Feel free to look at or pull my suggestion from my own forge here[^1]: https://lab.trax.im/fork/github/lae/ansible-role-proxmox/-/merge_requests/1/diffs or just edit it your own way, as you prefer.

I notice the reference to Ceph "pacific" is now also out of date, according to info in https://pve.proxmox.com/wiki/Package_Repositories -- so my suggested changes are incomplete. Looking further into this, defaults/main.yml says:

pve_ceph_repository_line: "deb http://download.proxmox.com/debian/{% if ansible_distribution_release == 'buster' %}ceph-nautilus buster{% else %}ceph-quincy bullseye{% endif %} main"

so looks like further updates may be needed there for bookworm?... I'm getting a bit out of my depth, I haven't used Ceph. I just hope these pointers may be of some help to you, if and when you might wish to look into it. I do not wish to put any pressure on you. It's just an issue, it's not meant to be a Big Issue! Thanks for considering it.

[^1]: git pull https://lab.trax.im/fork/github/lae/ansible-role-proxmox.git julianfoad-update-readme-bookworm