Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding buildctl task #11549

Merged
merged 15 commits into from
Oct 21, 2019
Merged

Conversation

prebansa
Copy link
Contributor

No description provided.

@prebansa prebansa requested a review from divman25 October 11, 2019 06:18
Tasks/BuildctlV0/task.loc.json Outdated Show resolved Hide resolved
Tasks/BuildctlV0/task.loc.json Outdated Show resolved Hide resolved
Tasks/BuildctlV0/task.loc.json Outdated Show resolved Hide resolved
Tasks/BuildctlV0/task.json Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/utils.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/utils.ts Outdated Show resolved Hide resolved
Tasks/BuildctlV0/src/utils.ts Outdated Show resolved Hide resolved
@shashankbarsin shashankbarsin self-assigned this Oct 12, 2019
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
@prebansa prebansa requested a review from vithati October 14, 2019 10:04
Tasks/ContainerBuildV0/task.json Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Show resolved Hide resolved
Tasks/ContainerBuildV0/src/dockerbuild.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/dockerbuild.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/docker.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/dockerpush.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/dockerpush.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/buildctl.ts Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/task.json Outdated Show resolved Hide resolved
Tasks/ContainerBuildV0/src/utils.ts Show resolved Hide resolved
Tasks/ContainerBuildV0/src/utils.ts Outdated Show resolved Hide resolved
tagArguments.push(imageName + ":" + tag);
});
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I had specified in the spec, if the user has explicitly provided the tags input, we honour that. Else, the default value of $(Build.BuildId) needs to be used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashankbarsin, This current behavior is same as other tasks. we should not default to $(Build.BuildId) if user gives empty string. If we always generates default tag, we will get issues from customer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today if user provides the following task input, $(Build.BuildId) is used for tagging the image because the task input has a default value of $(Build.BuildId) in task.json. And if the input is not provided a value, this default of $(Build.BuildId) is used.

- task: Docker@2
    inputs:
      containerRegistry: 'shasb'
      repository: 'shashankbarsin/playground'
      command: 'buildAndPush'
      Dockerfile: '**/Dockerfile'

In this context, which case is this else clause executed for? Only for the case where user explicitly mentions a tag input, but provides an empty string as value? Like so -

- task: Docker@2
    inputs:
      containerRegistry: 'shasb'
      repository: 'shashankbarsin/playground'
      command: 'buildAndPush'
      Dockerfile: '**/Dockerfile'
      tags: 

If that is the case, it's better to have consistency between the two cases I've mentioned above by using $(Build.BuildId) in both cases at least for this task as there's no issue of backward compatibility break

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today if user provides the following task input, $(Build.BuildId) is used for tagging the image because the task input has a default value of $(Build.BuildId) in task.json. And if the input is not provided a value, this default of $(Build.BuildId) is used.

  • task: Docker@2
    inputs:
    containerRegistry: 'shasb'
    repository: 'shashankbarsin/playground'
    command: 'buildAndPush'
    Dockerfile: '**/Dockerfile'
    In this context, which case is this else clause executed for?

Here input it self not specified so server sending the default input so it will tag with that.

Only for the case where user explicitly mentions a tag input, but provides an empty string as value? Like so -

  • task: Docker@2
    inputs:
    containerRegistry: 'shasb'
    repository: 'shashankbarsin/playground'
    command: 'buildAndPush'
    Dockerfile: '**/Dockerfile'
    tags:
    If that is the case, it's better to have consistency between the two cases I've mentioned above by using $(Build.BuildId) in both cases at least for this task as there's no issue of backward compatibility break

In this case user specified tags with empty input. I feel we should not tag anything. Adding $(Build.BuildId) is easy if customers reports they don't that behavior them removing is difficult. So I feel it's not better to tag if input is empty.

@vithati vithati dismissed divman25’s stale review October 21, 2019 04:45

Addressed all comments.

@prebansa prebansa merged commit 910fd1c into microsoft:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants