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

Update output file name #798

Merged
merged 1 commit into from
Jun 19, 2019
Merged

Conversation

iamhopaul123
Copy link
Contributor

@iamhopaul123 iamhopaul123 commented Jun 19, 2019

Issue #, if available:
#796
Description of changes:


Enter [N/A] in the box, if an item is not applicable to your change.

Testing

  • Unit tests passed
  • Integration tests passed
  • [N/A] Unit tests added for new functionality
  • [N/A] Listed manual checks and their outputs in the comments (example)
  • [N/A] Link to issue or PR for the integration tests:

Documentation

  • [N/A] Contacted our doc writer
  • [N/A] Updated our README

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iamhopaul123 iamhopaul123 requested a review from a team June 19, 2019 00:02
@hencrice
Copy link
Contributor

The original thread mentions that we have to update some code to use path.Join or os.PathSeparater. Are those changes done so that we can update project.go?

@iamhopaul123
Copy link
Contributor Author

The original thread mentions that we have to update some code to use path.Join or os.PathSeparater. Are those changes done so that we can update project.go?

The reason for using filepath.Join is to get the pwd and concat with the file name so that we can get the valid file path for every os system. However, the test case just want to test the ability to create the yml file so I think we don't need to provide the full path.

@hencrice
Copy link
Contributor

@iamhopaul123
Copy link
Contributor Author

https://github.com/aws/amazon-ecs-cli/blob/ecs-local/ecs-cli/modules/cli/local/project/project.go This is not just a test, right?

Well, I think I have to make some changes. Nice catch.

@iamhopaul123
Copy link
Contributor Author

https://github.com/aws/amazon-ecs-cli/blob/ecs-local/ecs-cli/modules/cli/local/project/project.go This is not just a test, right?

Do the latest changes look good to you?

LocalOutDefaultFileName = "./docker-compose.local.yml"
var (
wd, _ = os.Getwd()
LocalOutDefaultFileName = filepath.Join(wd, "docker-compose.local.yml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leverage this variable in local ps or local down? https://github.com/aws/amazon-ecs-cli/blob/ecs-local/ecs-cli/modules/cli/local/ps_app.go#L89

Removing code duplication, the main concern is that for whatever reason if we rename docker-compose.local.yml to something else we will need to remember to update it in multiple locations instead of just here.

Copy link
Contributor Author

@iamhopaul123 iamhopaul123 Jun 19, 2019

Choose a reason for hiding this comment

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

  1. Yeah I agree with you. I think that is doable and will have a try.

  2. "flags.LocalOutputFlag" is the string flag for customized yml name. If that flag is set, the default one will not be used. But I think we should get rid of "ecsLocalDockerComposeFileName" in create_app.go because this is not always the name we want.

@iamhopaul123
Copy link
Contributor Author

iamhopaul123 commented Jun 19, 2019

Manual Tests

Run the "create" command

ecs-cli local create --arn arn:aws:ecs:us-west-2:403971813171:task-definition/e2e-ec2-tutorial- taskdef:1

Successfully wrote docker-compose.local.yml

After that, the yml file is created at the working directory without the path concern for different os systems.

@iamhopaul123 iamhopaul123 merged commit 955f7e4 into aws:ecs-local Jun 19, 2019
@SoManyHs
Copy link
Contributor

HI @iamhopaul123,

In the future, please provide a more descriptive, concise title to your PR and include the "Fixes <issue number" in the commit message. That will allow Github to close the related issue automatically.

We probably should have this codified in our own contributing doc, but in the meantime you can take a look at the description for how to title your PRs here as an example.

@iamhopaul123
Copy link
Contributor Author

HI @iamhopaul123,

In the future, please provide a more descriptive, concise title to your PR and include the "Fixes <issue number" in the commit message. That will allow Github to close the related issue automatically.

We probably should have this codified in our own contributing doc, but in the meantime you can take a look at the description for how to title your PRs here as an example.

Hi @SoManyHs,

Thank you for your advice. I am sorry for this PR that might not be clear enough. And I will try to make the PR more descriptive in the future.

@SoManyHs SoManyHs added the ecs-local Issues related to the "local" subcommand. label Jul 9, 2019
@SoManyHs SoManyHs changed the title fixed #796 Update output file name Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecs-local Issues related to the "local" subcommand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants