Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-43016

BlueOcean editor: does not allow empty string for label on agent or node

    Details

    • Similar Issues:
    • Sprint:
      Blue Ocean 1.6 - beta 2, Blue Ocean - 1.6 - beta 4

      Description

      Node and agent label strings in Pipeline can be an empty string '', but BlueOcean editor does not allow it be blank (see attached).

        Attachments

          Issue Links

            Activity

            Hide
            nicu Nicolae Pascu added a comment -

            Andrew Bayer I am receiving the Label param from node as being required from jenkins/blue/rest/pipeline-metadata/agentMetadata?depth=20

            After talking to Vivek Pandey he said I should ping you about it.

            This is the full JSON response:
            https://gist.github.com/NicuPascu/9ed1d96d4604274683ad2ef48a0778fc

            Show
            nicu Nicolae Pascu added a comment - Andrew Bayer I am receiving the Label param from node as being required from jenkins/blue/rest/pipeline-metadata/agentMetadata?depth=20 After talking to Vivek Pandey  he said I should ping you about it. This is the full JSON response: https://gist.github.com/NicuPascu/9ed1d96d4604274683ad2ef48a0778fc
            Hide
            abayer Andrew Bayer added a comment -

            Yes, the label parameter is required for the label agent. But "" should be an acceptable value for it. That said, I don't think this is a must-fix - agent label with "" is literally the same thing as agent any in execution. Not just analogous - literally the same thing. So agent any is a perfectly acceptable alternative.

            Show
            abayer Andrew Bayer added a comment - Yes, the label parameter is required for the label agent. But "" should be an acceptable value for it. That said, I don't think this is a must-fix - agent label with "" is literally the same thing as agent any in execution. Not just analogous - literally the same thing. So agent any is a perfectly acceptable alternative.
            Hide
            nicu Nicolae Pascu added a comment -

            I understand your reasoning but in the front-end a required field means it should not be empty e.g. "". I suggest we either leave things as they are now or if we want an empty string to be a valid value in BO then I suggest we change the isRequired to false. Vivek Pandey what do you think?

            Show
            nicu Nicolae Pascu added a comment - I understand your reasoning but in the front-end a required field means it should not be empty e.g. "". I suggest we either leave things as they are now or if we want an empty string to be a valid value in BO then I suggest we change the isRequired to false. Vivek Pandey what do you think?
            Hide
            abayer Andrew Bayer added a comment -

            Yeah, I'm fine with leaving things as they are now. We're stuck with the required field bit for various historic reasons, but I don't think it's worth anyone's time to hack out a way to support empty string in the editor in this context when agent any achieves exactly the same thing.

            Show
            abayer Andrew Bayer added a comment - Yeah, I'm fine with leaving things as they are now. We're stuck with the required field bit for various historic reasons, but I don't think it's worth anyone's time to hack out a way to support empty string in the editor in this context when agent any achieves exactly the same thing.
            Hide
            bitwiseman Liam Newman added a comment - - edited

            I strongly disagree.  

            "agent { label '' }" is valid declarative syntax.  If I open a pipeline with that syntax in it, Blue Ocean will report an error and force me not to use that syntax. That is not acceptable.

            I understand that we'd like people to use "agent any" instead, but we should not be reporting an error in the editor for valid Declarative syntax.  

            It sounds like this might be exposing a design bug - there plenty of situations where required string is allowed to be empty.  Is there no way to specify "required but may be an empty string"? 

             

            Show
            bitwiseman Liam Newman added a comment - - edited I strongly disagree.   "agent { label '' }" is valid declarative syntax.  If I open a pipeline with that syntax in it, Blue Ocean will report an error and force me not to use that syntax. That is not acceptable. I understand that we'd like people to use "agent any" instead, but we should not be reporting an error in the editor for valid Declarative syntax.   It sounds like this might be exposing a design bug - there plenty of situations where required string is allowed to be empty.  Is there no way to specify "required but may be an empty string"?   
            Hide
            abayer Andrew Bayer added a comment -

            Aaaaah - yes, that error is bad. Didn't think of that. There is no way to do "required but can be empty (or null)". Probably the best bet would be to have the conversion to JSON just shrug and change to agent any when it hits that scenario.

            Show
            abayer Andrew Bayer added a comment - Aaaaah - yes, that error is bad. Didn't think of that. There is no way to do "required but can be empty (or null)". Probably the best bet would be to have the conversion to JSON just shrug and change to agent any when it hits that scenario.
            Hide
            bitwiseman Liam Newman added a comment -

            That sounds like a reasonable workaround to me.  

            Show
            bitwiseman Liam Newman added a comment - That sounds like a reasonable workaround to me.  
            Hide
            abayer Andrew Bayer added a comment -

            FYI, I've put up https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/264 on the Declarative side - this'll convert

            agent {
              label ''
            }
            

            to the JSON for agent any when translating to JSON for the editor, which will unbreak the most general use case for an empty string label. However, it'll still have an empty string label in the JSON if there are additional options, like in the case of

            agent {
              label ''
              customWorkspace '/some/path'
            }
            

            ...so even with this, the editor should still ideally have a way to specify an empty string for the label.

            Show
            abayer Andrew Bayer added a comment - FYI, I've put up https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/264 on the Declarative side - this'll convert agent { label '' } to the JSON for agent any when translating to JSON for the editor, which will unbreak the most general use case for an empty string label. However, it'll still have an empty string label in the JSON if there are additional options, like in the case of agent { label '' customWorkspace '/some/path' } ...so even with this, the editor should still ideally have a way to specify an empty string for the label.
            Hide
            jbriden Jenn Briden added a comment -

            Nicolae Pascu, I think that the approach that Liam Newman and Andrew Bayer support makes sense. In that case, I don't think we need any editor changes. Is that correct?

            Show
            jbriden Jenn Briden added a comment - Nicolae Pascu , I think that the approach that Liam Newman and Andrew Bayer support makes sense. In that case, I don't think we need any editor changes. Is that correct?
            Hide
            jbriden Jenn Briden added a comment -

            Andrew Bayer, with your change, do we need to do any updates to the frontend editor?

            Show
            jbriden Jenn Briden added a comment - Andrew Bayer , with your change, do we need to do any updates to the frontend editor?
            Hide
            abayer Andrew Bayer added a comment -

            I don’t believe so, no.

            Show
            abayer Andrew Bayer added a comment - I don’t believe so, no.
            Hide
            nicu Nicolae Pascu added a comment -

            Awesome, thanks Andrew Bayer, I will close this issue then.

            Show
            nicu Nicolae Pascu added a comment - Awesome, thanks Andrew Bayer , I will close this issue then.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java
            pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java
            pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/c18ba8695ff32a4cb81e711aaed5ee8a5718021e
            Log:
            JENKINS-43016 Convert empty string label agent to agent any in JSON

            This'll make the editor happier - the label agent type does require an
            argument, but an empty string is a valid (but weird) use case. The
            editor, however, has no way of distinguishing between "you didn't
            specify a label" and "you specified an empty string for the
            label". There still should be work in the editor to figure out how to
            actually handle this situation, but this fix at least will unbreak
            existing Jenkinsfiles with empty string label agents being opened in
            the editor, by converting the JSON in that case to be `agent any`,
            which is functionally equivalent if no additional options are specified.

            However, if additional options are specified (such as
            `customWorkspace`), we have to punt and keep using the empty string
            label agent.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy http://jenkins-ci.org/commit/pipeline-model-definition-plugin/c18ba8695ff32a4cb81e711aaed5ee8a5718021e Log: JENKINS-43016 Convert empty string label agent to agent any in JSON This'll make the editor happier - the label agent type does require an argument, but an empty string is a valid (but weird) use case. The editor, however, has no way of distinguishing between "you didn't specify a label" and "you specified an empty string for the label". There still should be work in the editor to figure out how to actually handle this situation, but this fix at least will unbreak existing Jenkinsfiles with empty string label agents being opened in the editor, by converting the JSON in that case to be `agent any`, which is functionally equivalent if no additional options are specified. However, if additional options are specified (such as `customWorkspace`), we have to punt and keep using the empty string label agent.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java
            pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java
            pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/c3dc1ea6badcabe2c3c383c561e1e986ea30e0e2
            Log:
            Merge pull request #264 from abayer/jenkins-43016

            JENKINS-43016 Convert empty string label agent to agent any in JSON

            Compare: https://github.com/jenkinsci/pipeline-model-definition-plugin/compare/3b74fc799c00...c3dc1ea6badc
            *NOTE:* This service been marked for deprecation: https://developer.github.com/changes/2018-04-25-github-services-deprecation/

            Functionality will be removed from GitHub.com on January 31st, 2019.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy http://jenkins-ci.org/commit/pipeline-model-definition-plugin/c3dc1ea6badcabe2c3c383c561e1e986ea30e0e2 Log: Merge pull request #264 from abayer/jenkins-43016 JENKINS-43016 Convert empty string label agent to agent any in JSON Compare: https://github.com/jenkinsci/pipeline-model-definition-plugin/compare/3b74fc799c00...c3dc1ea6badc * NOTE: * This service been marked for deprecation: https://developer.github.com/changes/2018-04-25-github-services-deprecation/ Functionality will be removed from GitHub.com on January 31st, 2019.

              People

              • Assignee:
                nicu Nicolae Pascu
                Reporter:
                bitwiseman Liam Newman
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: