Code review comment for ~mertkirpici/juju-lint:lp/2009536

I believe checking out the mp and discussions related to lp #1855858(linked to this mp) will clarify most of the questions. However let me try to answer them to the best of my understanding.

> Can rsyslog-forwarder-ha be deployed more than once in a machine?

Yes. See discussion around hyperconverged architecture in lp #1855858 also the status file provided by the reporter for lp #2009536(also linked).

 > If it can, I'm guessing that we will jump from KeyError to juju-lint error?

I don't fully understand this question. The reported bug has an attached juju status file where this already is the case. juju-lint is crashing with an unhandled KeyError when run against that status file with a ruleset that is shipped by the snap.

 > Don't we need to change the rules file to accept this corner case with rsyslog-forwarder-ha?

 That depends. We could resolve this issue by only making an addition like the following in the contrib/includes/base.yaml file:

 subordinates:
   rsyslog-forwarder-ha:
     allow-multiple: true # this fixes the unhandled exception
     where: ???

However this change would make rsyslog-forwarder-ha subordinate charm mandatory and we would need to specify where we expect to find this subordinate charm in the deployments using the "where" key.

The reason I did not go in that direction is because I realized that rsyslog-forwarder-ha is already listed as an optional operations subordinate in contrib/includes/operations.yaml file. Which, during runtime after every included yaml file is merged, will only end up in the top level "known charms" object but not in "subordinates" object(which is the cause of the unhandled KeyError in the first place).

Instead, this proposed solution completely discards the checks about duplicate existence of subordinates that are not declared as mandatory(in top level "subordinates" object).

This change already has a unit test written during handling of lp #1855858, which is called test_subordinate_duplicates_allow. Since the check was designed to run against a mandatory charm(nrpe), this issue was not caught. Coincidentally this also is the reason why coverage did not go down.

I do agree that although the coverage did not go down, this particular scenario is still not tested. Therefore I am planning to include this scenario as well for completeness' sake.