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

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

    XMLWordPrintable

Details

    • 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

            nicu Nicolae Pascu added a comment -

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

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

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

            nicu Nicolae Pascu added a comment - abayer I am receiving the Label param from node as being required from jenkins/blue/rest/pipeline-metadata/agentMetadata?depth=20 After talking to vivek  he said I should ping you about it. This is the full JSON response: https://gist.github.com/NicuPascu/9ed1d96d4604274683ad2ef48a0778fc
            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.

            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.
            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 what do you think?

            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 what do you think?
            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.

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

             

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

            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.
            bitwiseman Liam Newman added a comment -

            That sounds like a reasonable workaround to me.  

            bitwiseman Liam Newman added a comment - That sounds like a reasonable workaround to me.  
            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.

            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.
            jbriden Jenn Briden added a comment -

            nicu, I think that the approach that bitwiseman and abayer support makes sense. In that case, I don't think we need any editor changes. Is that correct?

            jbriden Jenn Briden added a comment - nicu , I think that the approach that bitwiseman and abayer support makes sense. In that case, I don't think we need any editor changes. Is that correct?
            jbriden Jenn Briden added a comment -

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

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

            I don’t believe so, no.

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

            Awesome, thanks abayer, I will close this issue then.

            nicu Nicolae Pascu added a comment - Awesome, thanks abayer , I will close this issue then.

            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.

            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.

            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.

            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

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

              Dates

                Created:
                Updated:
                Resolved: