sureshanaparti commented on pull request #4922:
URL: https://github.com/apache/cloudstack/pull/4922#issuecomment-836782326
> > 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.
@nathanmcgarvey I've split these changes across multiple commits, please
take a look.
Regarding your suggestions.
1. I have carefully checked the files that were renamed as part of this
exercise. The file name references and any changes to the command line
arguments are addressed, and the files exist immediately after merge.
2. Removed unrelated changes in
_plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/qemu/QemuImg.java_,
except the comments & typos: 'Snapshotss' at
_test/integration/component/test_acl_listsnapshot.py_ (minor changes and can
forget to correct this later, so ignoring this).
3. Updated the internal terminology to match with the other external / third
party product documentation (MySQL source / replicas, XenSever pool master,
etc).
4. Corrected the missing parts. Removed the changes here
_plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixSetupCommandWrapper.java_
, to be in sync with XenSever pool master and slave.
--
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]
