CLOUDSTACK-9957 Annotations #2181
Conversation
40e5ea6 to
ce1259b
Compare
|
@DaanHoogland @borisstoyanov Screenshots? |
There was a problem hiding this comment.
@DaanHoogland thanks, have shared some comments please see.
| import java.util.Date; | ||
|
|
||
| /** | ||
| * @since 4.11 |
There was a problem hiding this comment.
Comments not necessary, we can include the version field in API-command implementations
| import org.apache.cloudstack.api.response.ListResponse; | ||
|
|
||
| /** | ||
| * @since 4.11 |
| @@ -192,6 +191,8 @@ | |||
| public AlertService _alertSvc; | |||
| @Inject | |||
| public UUIDManager _uuidMgr; | |||
| @Inject | |||
| public AnnotationService annotationService; | |||
There was a problem hiding this comment.
I would avoid injecting annotationservice in baseCmd, but instead inject only in relevant API cmd classes.
There was a problem hiding this comment.
this is not the usual pattern on API classes. I am adhering to the general practice in this case
There was a problem hiding this comment.
As discussed, this is a breakage of pattern in the code base that goes two ways. Adhering to one and not the other has downsides either way. Not important now (in this PR) but worth revisiting is how to poetically describe used services per API without having to describe the samething over and over per set of API, i.e. the Annotation APIs.
| /** | ||
| * @since 4.11 | ||
| */ | ||
| @APICommand(name = AddAnnotationCmd.APINAME, description = "add an annotation.", responseObject = AnnotationResponse.class, |
There was a problem hiding this comment.
Remove since version comment, add version field in the APICommand annotation and add authorization to include admin and other user roles who should be able to access.
| */ | ||
| @APICommand(name = AddAnnotationCmd.APINAME, description = "add an annotation.", responseObject = AnnotationResponse.class, | ||
| requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) | ||
| public class AddAnnotationCmd extends BaseCmd{ |
There was a problem hiding this comment.
Fix styling, space after BaseCmd.
| * @since 4.11 | ||
| */ | ||
| public final class AnnotationManagerImpl extends ManagerBase implements AnnotationService, PluggableService { | ||
| public static final Logger s_logger = Logger.getLogger(AnnotationManagerImpl.class); |
There was a problem hiding this comment.
Consider modern variable naming? Let's use LOG or LOGGER ?
| List<AnnotationVO> annotations = | ||
| getAnnotationsForApiCmd(cmd); | ||
| List<AnnotationResponse> annotationResponses = | ||
| convertAnnotationsToResponses(annotations); |
There was a problem hiding this comment.
Unnecessary newlines may be removed.
| return addAnnotation(addAnnotationCmd.getAnnotation(), addAnnotationCmd.getEntityType(), addAnnotationCmd.getEntityUuid()); | ||
| } | ||
|
|
||
| public AnnotationResponse addAnnotation(String text, EntityType type, String uuid) { |
There was a problem hiding this comment.
General comment -- the API handling methods can use the events annotations that are used to create events when APIs are called. See other API such as (dynamic) roles APIs for example.
tools/apidoc/gen_toc.py
Outdated
| @@ -171,6 +171,9 @@ | |||
| 'StratosphereSsp' : ' Stratosphere SSP', | |||
| 'Metrics' : 'Metrics', | |||
| 'Infrastructure' : 'Metrics', | |||
| 'listAnnotations' : 'Annotations', | |||
There was a problem hiding this comment.
We can re-write this as:
'Annotations' : 'Annotations'
This will pick and group any apis under 'Annotations' that have the substring
| @@ -425,6 +425,8 @@ var dictionary = {"ICMP.code":"ICMP Code", | |||
| "label.allocated":"Allocated", | |||
| "label.allocation.state":"Allocation State", | |||
| "label.allow":"Allow", | |||
| "label.annotated.by":"Annotator", | |||
| "label.annotation":"Annotation", | |||
There was a problem hiding this comment.
If possible, should we say 'Last commented by' and 'Comment'. The word 'annotation' may be used to highlight some text, the feature here allows admins to put comments on entities/resources... is it too late to reconsider renaming the APIs and naming as comments?
There was a problem hiding this comment.
I do not see the gain in the rename. The word 'annotation' has connotations but so does 'comment'. Both are equally bad, IMHO.
8012dec to
0b8c8ae
Compare
| import org.apache.cloudstack.api.response.AnnotationResponse; | ||
|
|
||
| @APICommand(name = AddAnnotationCmd.APINAME, description = "add an annotation.", responseObject = AnnotationResponse.class, | ||
| requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.11", authorized = {RoleType.Admin}) |
There was a problem hiding this comment.
Do we want anyone to annotate their resources, say VMs? i.e. allow users?
There was a problem hiding this comment.
We might want to expand certainly for loadbalancers and maybe zones/pods/clusters. Not sure about VMs, yet. @PaulAngus has ideas on that as well
| @Override | ||
| public long getEntityOwnerId() { | ||
| // for now all annotations are belong to us | ||
| return Account.ACCOUNT_ID_SYSTEM; |
There was a problem hiding this comment.
Use CallContext to get calling user
| import org.apache.cloudstack.api.response.ListResponse; | ||
|
|
||
| @APICommand(name = ListAnnotationsCmd.APINAME, description = "Lists annotations.", responseObject = AnnotationResponse.class, | ||
| requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.11", authorized = {RoleType.Admin}) |
There was a problem hiding this comment.
Same as above, if we allow annotations to be created by users let allow them here.
There was a problem hiding this comment.
I don't see an above which is probably obfuscated by a rebase but as for this; we do not allow anybody but admins to annotate for now. When we find use cases we can add them here
| import org.apache.cloudstack.api.response.AnnotationResponse; | ||
|
|
||
| @APICommand(name = RemoveAnnotationCmd.APINAME, description = "remove an annotation.", responseObject = AnnotationResponse.class, | ||
| requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.11", authorized = {RoleType.Admin}) |
There was a problem hiding this comment.
same as above, please see ^^
| import java.util.Date; | ||
|
|
||
| /** | ||
| * @since 4.11 |
There was a problem hiding this comment.
The comment is useless, not necessary.
| import java.util.UUID; | ||
|
|
||
| /** | ||
| * @since 4.11 |
There was a problem hiding this comment.
The comment in not necessary.
|
|
||
| import java.util.List; | ||
|
|
||
| /** |
There was a problem hiding this comment.
The comment is not necessary.
| import java.util.List; | ||
|
|
||
| /** | ||
| * @since 4.1 |
There was a problem hiding this comment.
Remove this comment please, this is not necessary.
| } | ||
| static public boolean contains(String representation) { | ||
| try { | ||
| /* EntityType tiep = */ valueOf(representation); |
There was a problem hiding this comment.
Maybe remove comments throughout the code base that are may not be very useful or obvious
7f51909 to
0d5cee5
Compare
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0 Failed tests:
Skipped tests: Passed test suits: |
|
@DaanHoogland can you fix the conflicts, and address major review issues, thanks. |
16d4ec0 to
beb2daa
Compare
|
@blueorangutan package |
1 similar comment
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1130 |
|
LGTM |
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1572)
|
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1581)
|
this is a boiler plate feature for adding auditable annotations on entities in cloudstack. As of now Hosts are implemented. userVm, VRs/SystemVMs, Networks are examples that may follow.
see https://cwiki.apache.org/confluence/display/CLOUDSTACK/Annotations+on+entities for a high level description.