sureshanaparti commented on pull request #4922:
URL: https://github.com/apache/cloudstack/pull/4922#issuecomment-831731002
> Overall, and this is probably too late for this suggestion: I'd recommend
breaking this out into multiple commits: this is a huge single commit which
makes reverting, diffing, and really all repo maintenance hard.
>
> Detailed suggestions:
>
> 1. Carefully evaluate things things that are really version bump ideas
that could break external API compatibility:
> E.g. systemvm/debian/opt/cloud/bin/master.py →
systemvm/debian/opt/cloud/bin/router_mode.py
> a. Both the renaming of that file, and the changing of the command line
arguments.
> b. tools/docker/systemtpl.sh I hope those URLs and files actually
exist immediately after merge!
> 2. Remove unrelated changes to this PR... don't just tailgate stuff onto
an already huge commit:
>
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java
> plugins/hypervisors/ovm/src/main/java/com/cloud/ovm/object/Test.java
> test/integration/component/test_acl_listsnapshot.py partially
(Non-relevant typos: ("Snapshotss"))
> 3. Match internal terminology to other external project documentation:
> Examples:
> a. Instead of "backup/cloudBackup"... call "replica/cloudReplica" to
line up with MySQL docs post 8.0.23 (Ref:
https://dev.mysql.com/doc/refman/8.0/en/binlog-replication-configuration-overview.html)
> Found at least in these files:
> client/conf/db.properties.in
> framework/db/src/main/java/com/cloud/utils/db/TransactionLegacy.java
> .
> b. Instead of "Secondary" interface, which has a different meaning, how
about "member" or maybe not even changing it quite yet since most OS
documentation and Linux proper still actually calls them "slaves" in terms of
Linux channel-bonding, by definition. This is going to get really confusing
since if someone says 'secondary' interface, they usually mean a literal second
NIC or alias, not a subordinate interface in a channel-bond/team/whatever.
> Found at least in these files:
>
plugins/hypervisors/ovm3/src/test/java/com/cloud/hypervisor/ovm3/objects/NetworkTest.java
> scripts/vm/hypervisor/xenserver/cloud-setup-bonding.sh
>
...s/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
> 4. There are missing corrections if you were trying to be 100%
code-complete.
> Example:
> a.
...va/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixSetupCommandWrapper.java
still has "slave" as a passed method parameter.
@nathanmcgarvey Thanks for the review, I'll update the changes accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
