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

Add missing run_tag directive. #1995

Merged
merged 1 commit into from
Jan 9, 2022

Conversation

psigen
Copy link
Contributor

@psigen psigen commented Jan 8, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

The docker run command fails because it is missing a run_tag arg due to a missing line merged in #1957.

Issue Number: N/A

What is the new behavior?

The docker run command succeeds and runs as per documentation.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

The line "%{run_tag}": run_tag, was omitted during this merge commit:
922e466#diff-2a6edca3d784a368586063a7774b7551bbf4476c7345c511ab987bf95c610527L281

Referenced in this comment on the original PR:
#1957 (comment)

@psigen psigen changed the title Add missing run_tag directive. Add missing run_tag directive. Jan 8, 2022
@psigen
Copy link
Contributor Author

psigen commented Jan 8, 2022

This might need to be merged quickly to unbreak master, but I would really love to add a test for this in a followup PR. Is there a good place in testing/e2e?

@alexeagle
Copy link
Collaborator

+1 on adding some missing e2e for this, thank you.
I'm not sure where that goes.

Will merge this now to unbreak HEAD, thanks for explaining your analysis of what happened.

@alexeagle alexeagle merged commit 40d4036 into bazelbuild:master Jan 9, 2022
@psigen
Copy link
Contributor Author

psigen commented Jan 9, 2022

Just confirmed that behavior is as expected post-merge on master right now.

Tested bazel run on a go_image target with and without --norun flag and passed some volume and env flags to the docker daemon with expected results.

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.

2 participants