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

fix(docker_context): enable if either yml or yaml is found #2053

Merged
merged 2 commits into from Jan 11, 2021
Merged

fix(docker_context): enable if either yml or yaml is found #2053

merged 2 commits into from Jan 11, 2021

Conversation

benwaffle
Copy link
Contributor

@benwaffle benwaffle commented Jan 3, 2021

Description

If only_with_files = true is set for the docker_context module, then this prompt is only shown in case a Dockerfile or docker-compose.yml file is found.
This PR also enables it for docker-compose.yaml (notice yml vs yaml).

Motivation and Context

Fixes the prompt for a valid docker-compose yaml file.

How Has This Been Tested?

  1. Created a docker-compose.yaml file
  2. Ran ~/path/to/dev/build/target/starship prompt docker_context
  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Copy link
Member

@andytom andytom left a comment

Choose a reason for hiding this comment

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

chipbuster
chipbuster previously approved these changes Jan 9, 2021
Copy link
Contributor

@chipbuster chipbuster left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable change. Thanks for your contribution!

@matchai
Copy link
Member

matchai commented Jan 9, 2021

Along with an addition to docs, it would be good if we had a test to cover this new code path.

@chipbuster chipbuster dismissed their stale review January 9, 2021 04:49

Forgot about docs

@benwaffle
Copy link
Contributor Author

I'm not sure how to write the test.

@chipbuster
Copy link
Contributor

I was going to suggest following the existing tests for the docker_context module, but....we have no tests in there.

Why don't we have tests for the docker module? I'm very tired right now so it's possible I'm just forgetting where they are, but if it turns out that we don't have tests for the docker module, I'd be in favor of merging this PR as-is and then adding tests in a separate PR (or as part of another issue).

@andytom
Copy link
Member

andytom commented Jan 11, 2021

I was going to suggest following the existing tests for the docker_context module, but....we have no tests in there.

Why don't we have tests for the docker module? I'm very tired right now so it's possible I'm just forgetting where they are, but if it turns out that we don't have tests for the docker module, I'd be in favor of merging this PR as-is and then adding tests in a separate PR (or as part of another issue).

It looks like there are no test for this module. Let's get this merged in and I'll create a separate issue for adding tests.

@andytom andytom merged commit 0a9db85 into starship:master Jan 11, 2021
@andytom
Copy link
Member

andytom commented Jan 11, 2021

Thank you for your contribution @benwaffle

chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
…2053)

* fix(docker_context): enable if either yml or yaml is found

* Update docs
@benwaffle benwaffle deleted the docker-context-yaml branch January 20, 2021 03:50
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.

None yet

4 participants