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

[tasks] Include Names for include_tasks #226

Merged

Conversation

the-real-cphillips
Copy link
Contributor

@the-real-cphillips the-real-cphillips commented Nov 21, 2019

Currently, the output when running this role is confusing.
It isn't clear what exactly is being skipped for each include_tasks play.

This was initially confusing until I dug into the actual tasks/main.yml
This PR adds a name: parameter to each include_tasks within the tasks/main.yml

While a small nit-picky PR, It makes the output clearer and easier to explain to others, without having to dive into code.

Currently, the output when running this agent is not clear what exactly
is being skipped.

This just adds `name:` to each `include_tasks` in `tasks/main.yml`
This makes the output a little more friendly.
@KSerrania
Copy link
Contributor

Hello @the-real-cphillips,

Thanks for the PR! We released a new major version of the role this Wednesday, so the task list got modified. Would you mind rebasing your PR and adding the name: parameter to the new and modified tasks?

@the-real-cphillips
Copy link
Contributor Author

Hello @the-real-cphillips,

Thanks for the PR! We released a new major version of the role this Wednesday, so the task list got modified. Would you mind rebasing your PR and adding the name: parameter to the new and modified tasks?

Sure thing! I'll get on it now :)

Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

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

Thanks! I'm adding some suggested changes to make the role work as expected, the merge reverted some changes that were done in the major version bump (mainly some filename changes, and datadog_agent5 got replaced by datadog_agent_major_version).

Once these are added the PR should be good to go!

tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
Co-Authored-By: Kylian Serrania <[email protected]>
Co-Authored-By: Kylian Serrania <[email protected]>
Co-Authored-By: Kylian Serrania <[email protected]>
Co-Authored-By: Kylian Serrania <[email protected]>
Co-Authored-By: Kylian Serrania <[email protected]>
Co-Authored-By: Kylian Serrania <[email protected]>
- missed the `agent6-linux.yml` to `agent-linux.yml`, updated here
- ran through `yamllint` and `ansible-lint`
  - no issues except for `ansible-lint` errors for 501/602
Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

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

Thanks!
I'm going to merge this PR and release a new version on Monday.

As you mentioned in your commits, we have a few linting errors, but they're tackled by #236 so everything's good here!

@the-real-cphillips
Copy link
Contributor Author

Thanks!
I'm going to merge this PR and release a new version on Monday.

As you mentioned in your commits, we have a few linting errors, but they're tackled by #236 so everything's good here!

Awesome thanks!

@the-real-cphillips
Copy link
Contributor Author

@KSerrania probably unrelated here, but has there been any talk about including molecule for testing the role to ensure it's idempotent and works as expected?

I plausibly would be willing to undertake such a thing if there's desired need/want for it.

@KSerrania KSerrania merged commit 7fdb06d into DataDog:master Dec 23, 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.

2 participants