You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/10/20 13:35:49 UTC

[GitHub] [skywalking-infra-e2e] mrproliu opened a new pull request #58: Let reuse file support environment to replace

mrproliu opened a new pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58


   We have some similar cases in the main repo, such as verifying the service metrics, they have multiple metrics and services need to be verified. 
   
   So we could use the environment to replace the reuse file content, to help users reduce the cases and help read the cases. 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947733726


   We should not keep everyone configurable, because that basically we create another language. The benefit of a tool is providing clear use guidelines and easier to understand. 
   This feature makes the `cases.yaml` becoming `cases.command.template`, then other contributors have to read the `e2e.yaml`. When they want to do local debug, they don't have a direct command copy/paste. Manually assembling command increases the bar.
   We wouldn't change these conditions AFAIK, as most of those are either important default metrics in SkyWalking or only names relating to demo services. 
   These are typical side-effects.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732800868



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -116,6 +116,8 @@ verify:
       expected: path/to/expected.yaml   # excepted content file path
     - includes:      # including cases
         - path/to/cases.yaml            # cases file path
+      environment:                      # include cases with environment

Review comment:
       ```suggestion
         environments:                      # Available system environment key:value pairs
   ```




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732800868



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -116,6 +116,8 @@ verify:
       expected: path/to/expected.yaml   # excepted content file path
     - includes:      # including cases
         - path/to/cases.yaml            # cases file path
+      environment:                      # include cases with environment

Review comment:
       ```suggestion
         environments:                      # include cases with environment
   ```




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] mrproliu closed pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
mrproliu closed pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58


   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947744440


   Another typically prove usually I would like to refer it. Check my comments about your documents, and think we could say this feature clearly, and guide users to use. I am feeling you are struggle when you were writing this document. 
   Even with the example, it is not very clear.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947695942


   > We have some similar cases in the main repo, such as verifying the service metrics, they have multiple metrics and services need to be verified.
   
   First, I am not objecting this feature, but I want to confirm, do we(SkyWalking main repo) really need this? We have 2 services basically in the e2e tests mostly, so, in general, only 2 groups of verification(duplicate) we should do. 
   Is this your expectation to use one group of general verification files? Moving parameters out of those files? 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947729188


    My argument about the query command templatization would be, what is the real exact different of these two. 
   Simply `swctl` command is easier for new contribute to use, because the document is consistent and provided in CLI already.
   Another template tech makes me feeling over-engineer.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng edited a comment on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947729188


    My argument about the query command templatize would be, what is the real exact different of these two. 
   Simply `swctl` command is easier for new contribute to use, because the document is consistent and provided in CLI already.
   Another template tech makes me feeling over-engineer.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] mrproliu commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
mrproliu commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-948200219


   Ok, I think this future is not good to use and can't help to reduce too many cases. So I closed it.  


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732806184



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -193,6 +195,28 @@ cases:
      expected: path/to/expected.yaml   # excepted content file path
 ```
 
+#### Environment
+
+You could be using `environment` to replace variables from include cases. 
+In the include file, you could declare `${var}` or `$var` to show which environment need to be replaced.
+
+```yaml
+# verify file
+- include:
+    - path/to/include
+  environment:
+    # The key and value only accept string value
+    customKey: customValue  
+
+# include file
+cases:
+  - query: echo "$customKey"
+  
+# Executing verify case, replace environment data
+cases:
+   - query: echo "customValue"
+```

Review comment:
       I think this is not a replacement. This is a variable declaration in `case file` and use your new config as value supplier?




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947784001


   For Istio cases, actually you don't need to verify all services' metrics. That is not the point of e2e. From ALS analysis logic, check ingress and two other services should be enough. Even in Istio case, you only need to write 5 services' verification in one time, not repeatedly. 
   I think this is not a good reason to support a one-time writing through a 205 LOC feature. This feature is not good supported by real use case.
   I know this is not go template, that is also another concern. A new and not widely used template tech is making things harder. 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732800868



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -116,6 +116,8 @@ verify:
       expected: path/to/expected.yaml   # excepted content file path
     - includes:      # including cases
         - path/to/cases.yaml            # cases file path
+      environment:                      # include cases with environment

Review comment:
       ```suggestion
         environments:                      # Available system environment key:value pairs, which could override values of declared variable in `including cases` files.
   ```




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] mrproliu commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
mrproliu commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947768939


   Sorry for the late reply. I think we just need to use the environment and make them as reuse file variable value suppliers. Don't need to use the `go` template, it's more different to use. 
   I found the `istio` case is the main repo is different, it needs to bootstrap 5 services, so we need to verify their metrics, maybe it's extreme. Maybe it does not help to reduce so many cases to write, but it's helpful for users to read the `e2e.yaml` and reduce the chance of errors or changes due to repetition. 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng edited a comment on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947733726


   We should not keep everything configurable, because that basically we create another language. The benefit of a tool is providing clear use guidelines and easier to understand. 
   This feature makes the `cases.yaml` becoming `cases.command.template`, then other contributors have to read the `e2e.yaml`. When they want to do local debug, they don't have a direct command copy/paste. Manually assembling command increases the bar.
   We wouldn't change these conditions AFAIK, as most of those are either important default metrics in SkyWalking or only names relating to demo services. 
   These are typical side-effects.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947743220


   @mrproliu 
   We are going to market this tool, at least I and you will. Basically, after our main immigration, I will talk about this repeatedly in all SkyWalking general sessions. 
   In those session, I want to provide them how we use, because this is so-call official, which should be straight forward.
   
   I am not feeling bad for this feature(from tech perspective), but I don't want our recommend cases(main repo's) using this.
   
   Hope this context makes clear background about why I am arguing about don't provide this.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] kezhenxu94 edited a comment on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947738722


   > then other contributors have to read the `e2e.yaml`. When they want to do local debug, they don't have a direct command copy/paste. Manually assembling command increases the bar.
   
   That's not the case, the logs will print the final rendered commands. But I agree and don't either want to abstract the configuration too much to a fault, for the main reason @mrproliu propose this feature for, I feel it's just OK if we don't have this feature. 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732803610



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -116,6 +116,8 @@ verify:
       expected: path/to/expected.yaml   # excepted content file path
     - includes:      # including cases
         - path/to/cases.yaml            # cases file path
+      environment:                      # include cases with environment

Review comment:
       I guess this is what your mean? If so, please update document 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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] kezhenxu94 commented on a change in pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#discussion_r732813412



##########
File path: docs/en/setup/Configuration-File.md
##########
@@ -193,6 +195,28 @@ cases:
      expected: path/to/expected.yaml   # excepted content file path
 ```
 
+#### Environment
+
+You could be using `environment` to replace variables from include cases. 
+In the include file, you could declare `${var}` or `$var` to show which environment need to be replaced.
+
+```yaml
+# verify file
+- include:
+    - path/to/include
+  environment:
+    # The key and value only accept string value
+    customKey: customValue  
+
+# include file
+cases:
+  - query: echo "$customKey"
+  
+# Executing verify case, replace environment data
+cases:
+   - query: echo "customValue"
+```

Review comment:
       Bear in mind the doc will serve as a guideline and demonstrate to users the typical use case of this feature, so you'd better mock some "real" scenarios instead of giving random configs like `echo $customKey`, `echo "customValue"`




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] wu-sheng edited a comment on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
wu-sheng edited a comment on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947733726


   We should not keep everything configurable, because that basically we create another language. The benefit of a tool is providing clear use guidelines and easy to understand. 
   This feature makes the `cases.yaml` becoming `cases.command.template`, then other contributors have to read the `e2e.yaml`. When they want to do local debug, they don't have a direct command copy/paste. Manually assembling command increases the bar.
   We wouldn't change these conditions AFAIK, as most of those are either important default metrics in SkyWalking or only names relating to demo services. 
   These are typical side-effects.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking-infra-e2e] kezhenxu94 commented on pull request #58: Let reuse file support environment to replace variable

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #58:
URL: https://github.com/apache/skywalking-infra-e2e/pull/58#issuecomment-947738722


   > then other contributors have to read the `e2e.yaml`. When they want to do local debug, they don't have a direct command copy/paste. Manually assembling command increases the bar.
   
   That's not the case, the logs will print the final rendered commands. But I agree and don't either want to abstract the configuration too much to a fault, for the main reason @mrproliu propose this feature for, I feel it's just OK.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org