You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/01 13:06:17 UTC

[GitHub] [cloudstack] RodrigoDLopez opened a new pull request #4194: enable update tags on disk offerings

RodrigoDLopez opened a new pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   ## Description
   <!--- Describe your changes in detail -->
   
   On this PR [#2636]((https://github.com/apache/cloudstack/pull/2636)), @rafaelweingartner proposed a consistent validation mechanism to select a storage pool to deploy a volume when a virtual machine is deployed or when a new data disk is allocated.
   The following table presents the scenarios when the storage_pool supports or not a new data disk.
   
   | #	| Disk offering tags	| Storage tags	| Does the storage support the disk offering?	|
   |---| ---					| ---			| ---											|
   | 1	| A,B					| A				| NO											|
   | 2	| A,B,C					| A,B,C,D,X		| YES											|
   | 3	| A,B,C					| X,Y,Z			| NO											|
   | 4	| null					| A,S,D			| YES											|
   | 5	| A						| null			| NO											|
   | 6	| null					| null			| YES											|
   
   
   With that in mind, this PR enables `updateDiskOfferingCmd` to edit storage pool tags for `diskoffering`. Thus, providing greater control to ADMINS decide where disks will be allocated.
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [x] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## Screenshots (if appropriate):
   ![Screenshot from 2020-06-30 14-51-39](https://user-images.githubusercontent.com/19981369/86246469-694c3a00-bb81-11ea-9c62-484fa2e56704.png)
   
   ## How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   To test the PR i created 2 storage pools.
   Storage Pools:
   - spool01 	        with tag hdd
   - spool02		with tag hdd2
   
   And create one disk Offering:
   - Test 		without tag
   
   #### Test 01
   - Create a volume using Test diskoffering
   - Attach the volume to an instance
   - Update the disk's diskoffering to another one that has a tag that does not have a corresponding Storage pool
   - Detach the volume
   - try to migrate the disk
   
   **Expected:** we expect an exception since no storage tags match with the diskoffering tag
   **Result:** 
   ```
   Migration target pool [null, tags:null] has no matching tags for volume [small, uuid:ce026b50-8b04-44d6-bb66-3d324b64ca7d, tags:fake]
   ```
   
   #### Test 02
   - Using the same volume from Test 01
   - Update the disk's diskoffering tag to hdd2
   - try to migrate the disk to spool02
   
   **Expected:** Migrate the volume to spool02 storage.
   **Result:** all good, as expected
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-669091806


   If enough OKs and tests on this I won't stand in the way, but I am very weary of changing tags of an offering that is in use. Would this hamper a restart/redeploy for instance. cc @andrijapanicsb @rhtyd @PaulAngus what do you think?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-697915201


   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-709842831


   <b>Trillian test result (tid-2973)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 39213 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4194-t2973-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 84 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_hostha_kvm_host_fencing | `Error` | 174.80 | test_hostha_kvm.py
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r448483662



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateDiskOfferingCmd.java
##########
@@ -77,6 +77,13 @@
             since = "4.13")
     private String zoneIds;
 
+    @Parameter(name = ApiConstants.TAGS,
+            type = CommandType.STRING,
+            description = "Comma-separated list of tags to be added to disk offering",

Review comment:
       i will improve this description




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] GabrielBrascher commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-669167456


   @DaanHoogland (with all due respect :slightly_smiling_face:) I share the same idea as @andrijapanicsb. I think that admins rather have control and flexibility (at their own risks) than having too many  barriers to keep them "safe". 
   
   In this particular case, if the system operator configures the wrong tag, it would raise an exception when deploying a VM/Volume saying that failed to find storage with the respective tag(s). Operators would then see the log, and fix it via API (not via DB)
   
   I totally agree that it is our job to avoid bugs and critical issues, but in my experience sometimes we avoid issues up to a point that system admins cannot operate the way they need and at the end, it results on workarounds and SQL queries to do things that we could do via API on a safer way.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rafaelweingartner commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-697781688


   @DaanHoogland is there a reason for us to close this PR? 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-668942686


   @GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] weizhouapache commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r453673201



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateDiskOfferingCmd.java
##########
@@ -77,6 +77,13 @@
             since = "4.13")
     private String zoneIds;
 
+    @Parameter(name = ApiConstants.TAGS,
+            type = CommandType.STRING,
+            description = "comma-separated list of tags for the disk offering, tags should match with existing storage pool tags",
+            since = "4.15")
+    private String tags;

Review comment:
       authorized = {RoleType.Admin}
   
   ?
   
   




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] andrijapanicsb commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-669110017


   @DaanHoogland if user doesn't know what he is doing, that's his "problem" - we give the oportunity to change it (instead of hacking the DB, that users tend to do these days).
   
   And no, it will not affect deployment of volumes - once the volume is created on the proper (based on tags) storage, tag's are never evaluated again for them (only evaluated when new volumes are created).


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689870180


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-685506856


   @blueorangutan package
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161345


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-682454137


   Packaging result: ✖centos7 ✖centos8 ✔debian. JID-1842


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r453699456



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateDiskOfferingCmd.java
##########
@@ -77,6 +77,13 @@
             since = "4.13")
     private String zoneIds;
 
+    @Parameter(name = ApiConstants.TAGS,
+            type = CommandType.STRING,
+            description = "comma-separated list of tags for the disk offering, tags should match with existing storage pool tags",
+            since = "4.15")
+    private String tags;

Review comment:
       code has been updated, thanks for the hint @weizhouapache 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684566421


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-682423929


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-687650274


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161345






----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-663772676


   can I get some reviews here, please?


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r448413252



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/offering/UpdateDiskOfferingCmd.java
##########
@@ -77,6 +77,13 @@
             since = "4.13")
     private String zoneIds;
 
+    @Parameter(name = ApiConstants.TAGS,
+            type = CommandType.STRING,
+            description = "Comma-separated list of tags to be added to disk offering",

Review comment:
       > Comma-separated list of tags to be added to disk offering
   
   What do you think of changing this description to somethings as:
   _"comma-separated list of tags for the disk offering, tags should match with existing storage pool tags"_

##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){
+            if(tags.isBlank()){
+                diskOffering.setTags(null);
+            }else {
+                diskOffering.setTags(tags);
+            }
+        }

Review comment:
       Can you please update the code formation according to the [CloudStack coding conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions)?
   
   Example of code formated:
   ```
   if (tags != null) {
       if (tags.isBlank()) {
           diskOffering.setTags(null);
       } else {
           diskOffering.setTags(tags);
       }
   }
   ```
   
   Documentation: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rafaelweingartner commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r461945911



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3183,7 +3184,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by id user: %s because it is not root-admin or domain-admin", diskOfferingHandle.getUuid(), user.getUuid()));
         }
 
-        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null;
+        final boolean updateNeeded = isUpdateDiskOfferingNeeded(name, displayText, sortKey, displayDiskOffering, tags);

Review comment:
       `shouldUpdateDiskOffering` sounds better to me. However, I am not a native speaker. Therefore, change only if you also agree with me.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-682423929


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161345


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-682424488


   @blueorangutan package
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-697940930


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2072


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684742680


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r461859143



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3183,7 +3184,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by id user: %s because it is not root-admin or domain-admin", diskOfferingHandle.getUuid(), user.getUuid()));
         }
 
-        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null;
+        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null || tags != null;

Review comment:
       I extract this to an method, and add documentation for this new method

##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){
+            if(tags.isBlank()){

Review comment:
       done... thanks for the hint




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689501138


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-652637451


   @RodrigoDLopez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] andrijapanicsb commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-669174222


   (that boils down to "don't give a sharp knife to a kid, but let wife take the sharp knife to prepare a meal" - whoever is using it should be proficient ;) )


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698675719


   <b>Trillian test result (tid-2845)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 64649 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4194-t2845-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 81 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_deploy_kubernetes_ha_cluster | `Error` | 3626.05 | test_kubernetes_clusters.py
   test_04_deploy_and_upgrade_kubernetes_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   test_05_deploy_and_upgrade_kubernetes_ha_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   test_06_deploy_and_invalid_upgrade_kubernetes_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_07_deploy_and_scale_kubernetes_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 37.76 | test_kubernetes_clusters.py
   ContextSuite context=TestCreateVolume>:setup | `Error` | 0.00 | test_volumes.py
   ContextSuite context=TestVolumes>:setup | `Error` | 0.00 | test_volumes.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Error` | 938.19 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Error` | 348.27 | test_vpc_redundant.py
   test_04_rvpc_network_garbage_collector_nics | `Error` | 3912.53 | test_vpc_redundant.py
   test_hostha_kvm_host_fencing | `Error` | 101.18 | test_hostha_kvm.py
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684679447


   Packaging result: ✖centos7 ✖centos8 ✔debian. JID-1863


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r479010824



##########
File path: ui/scripts/configuration.js
##########
@@ -2939,7 +2940,41 @@
                                         label: 'label.cache.mode'
                                     },
                                     tags: {
-                                        label: 'label.storage.tags'
+                                        label: 'label.storage.tags',
+                                        docID: 'helpPrimaryStorageTags',
+                                        isEditable: true,
+                                        isTokenInput: true,
+                                        dataProvider: function(args) {
+                                            $.ajax({
+                                                url: createURL("listStorageTags"),
+                                                dataType: "json",
+                                                success: function(json) {
+                                                    var item = json.liststoragetagsresponse.storagetag;

Review comment:
       @RodrigoDLopez does this require any changes in Primate?
   Can you check and send a PR to Primate? Thnx




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-668955902


   Packaging result: ✖centos7 ✔debian. JID-1650


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161662


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698781929


   these are to many errors, trying insanity
   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rafaelweingartner commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r460404337



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3183,7 +3184,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             throw new InvalidParameterValueException(String.format("Unable to update disk offering: %s by id user: %s because it is not root-admin or domain-admin", diskOfferingHandle.getUuid(), user.getUuid()));
         }
 
-        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null;
+        final boolean updateNeeded = name != null || displayText != null || sortKey != null || displayDiskOffering != null || tags != null;

Review comment:
       Can you extract this `name != null || displayText != null || sortKey != null || displayDiskOffering != null || tags != null` to a method? Then, you can document it (explain what it checks, and why), and after that, you can write unit tests for all of the cases that the method must return true/false.
   
   Moreover, instead of checking if it is different from null, you could use `String.Utils.isNotBlank`

##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){

Review comment:
       Use the String utils here, then you do not need two ifs

##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){
+            if(tags.isBlank()){

Review comment:
       So, whenever updating the offering, we need to send the old(already configured) tags? Even when we do not want to update them?
   
   Otherwise, it seems that you would always set it to null (even if we do not want that)




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-687650083


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684741864


   @blueorangutan package
   
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689886267


   Packaging result: ✖centos7 ✖centos8 ✔debian. JID-1942


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161662






----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-682424861


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-652653096


   Packaging result: ✔centos7 ✔debian. JID-1501


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698675719


   <b>Trillian test result (tid-2845)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 64649 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4194-t2845-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
   Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 81 look OK, 4 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_deploy_kubernetes_ha_cluster | `Error` | 3626.05 | test_kubernetes_clusters.py
   test_04_deploy_and_upgrade_kubernetes_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   test_05_deploy_and_upgrade_kubernetes_ha_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   test_06_deploy_and_invalid_upgrade_kubernetes_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_07_deploy_and_scale_kubernetes_cluster | `Error` | 0.05 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 37.76 | test_kubernetes_clusters.py
   ContextSuite context=TestCreateVolume>:setup | `Error` | 0.00 | test_volumes.py
   ContextSuite context=TestVolumes>:setup | `Error` | 0.00 | test_volumes.py
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Error` | 938.19 | test_vpc_redundant.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Error` | 348.27 | test_vpc_redundant.py
   test_04_rvpc_network_garbage_collector_nics | `Error` | 3912.53 | test_vpc_redundant.py
   test_hostha_kvm_host_fencing | `Error` | 101.18 | test_hostha_kvm.py
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698782441


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698161662


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-652637180


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] GabrielBrascher commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-668942428






----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689870351


   @rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689500856


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684565089


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-709534050


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-686907619






----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-697913876


   > @DaanHoogland is there a reason for us to close this PR?
   
   other than me making a mistake, @rafaelweingartner ?
   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-684790753


   Packaging result: ✔centos7 ✖centos8 ✔debian. JID-1869


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698782441


   @DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd closed pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-652665127


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-709533453


   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-687655564


   Packaging result: ✖centos7 ✖centos8 ✖debian. JID-1900


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-694921861


   @blueorangutan package


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r479778348



##########
File path: ui/scripts/configuration.js
##########
@@ -2939,7 +2940,41 @@
                                         label: 'label.cache.mode'
                                     },
                                     tags: {
-                                        label: 'label.storage.tags'
+                                        label: 'label.storage.tags',
+                                        docID: 'helpPrimaryStorageTags',
+                                        isEditable: true,
+                                        isTokenInput: true,
+                                        dataProvider: function(args) {
+                                            $.ajax({
+                                                url: createURL("listStorageTags"),
+                                                dataType: "json",
+                                                success: function(json) {
+                                                    var item = json.liststoragetagsresponse.storagetag;

Review comment:
       Hi @rhtyd I believe yes, some changes will be necessary on Cloud-Primate
   I will take a look on this project, but i'm not familiarized with VUE.js




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] rafaelweingartner commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r460517231



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){
+            if(tags.isBlank()){

Review comment:
       I see what you did now. I created a method to remove the tags by sending blank spaces. Can you extract this to a method? Then, document it, and create unit tests? Also, we need an end-user documentation for this one.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-698781929


   these are to many errors, trying insanity
   @blueorangutan test


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r448483807



##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
             diskOffering.setDisplayOffering(displayDiskOffering);
         }
 
-        // Note: tag editing commented out for now;keeping the code intact,
-        // might need to re-enable in next releases
-        // if (tags != null)
-        // {
-        // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
-        // {
-        // //no new tags; no existing tags
-        // diskOffering.setTagsArray(csvTagsToList(null));
-        // }
-        // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
-        // null)
-        // {
-        // //new tags + existing tags
-        // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
-        // List<String> newTags = csvTagsToList(tags);
-        // oldTags.addAll(newTags);
-        // diskOffering.setTagsArray(oldTags);
-        // }
-        // else if(!tags.trim().isEmpty())
-        // {
-        // //new tags; NO existing tags
-        // diskOffering.setTagsArray(csvTagsToList(tags));
-        // }
-        // }
+        if (tags != null){
+            if(tags.isBlank()){
+                diskOffering.setTags(null);
+            }else {
+                diskOffering.setTags(tags);
+            }
+        }

Review comment:
       done, fixed checkstyle issue




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland merged pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4194: enable update tags on disk offerings

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#issuecomment-689532786


   Packaging result: ✔centos7 ✖centos8 ✖debian. JID-1936


----------------------------------------------------------------
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:
users@infra.apache.org