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 Tomcat plugin. #2447

Closed
wants to merge 0 commits into from
Closed

Adding Tomcat plugin. #2447

wants to merge 0 commits into from

Conversation

mlindes
Copy link
Contributor

@mlindes mlindes commented Feb 20, 2017

This plugin resolves open issue #763 (#763)

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@mlindes
Copy link
Contributor Author

mlindes commented Apr 28, 2017

Any chance this will make the 1.3 release?

@danielnelson
Copy link
Contributor

No, I released a rc for 1.3 yesterday: #2733.

@nitin02
Copy link

nitin02 commented Jul 6, 2017

Hi, Is this plugin release added to the next milestone? Which version will this plugin be available?

@toni-moreno
Copy link
Contributor

We would like to test it!!

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Looks great, can you fix one issue in the toml before I merge?

# Cedentials for status URI.
# Default is tomcat/s3cret.
username = tomcat
password = s3cret
Copy link
Contributor

Choose a reason for hiding this comment

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

These strings need to be quoted so they are valid toml.


### Measurements & Fields:

<optional description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

## Cedentials for status URI.
## Default is tomcat/s3cret.
username = tomcat
password = s3cret
Copy link
Contributor

Choose a reason for hiding this comment

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

Quote toml strings here too.

@danielnelson danielnelson added this to the 1.4.0 milestone Jul 26, 2017
@mlindes
Copy link
Contributor Author

mlindes commented Jul 28, 2017

Quotes added. CircleCI complained about a zookeeper task. Not sure that's related to this pr.. I can follow-up if it is, though.

@danielnelson
Copy link
Contributor

I think if you rebase it will fix the zookeeper issue.

@mlindes
Copy link
Contributor Author

mlindes commented Jul 31, 2017

I don't really see how a rebase would solve this CircleCI error:

sudo service zookeeper stop
zookeeper: unrecognized service
sudo service zookeeper stop returned exit code 1
Action failed: sudo service zookeeper stop

Can you re-trigger the build to try it again? Permissions don't allow me to do that.
Otherwise, which tag would you like me to rebase off of. The first (successful) build was built from the 1.2.1 tag - which was the most current at the time of the PR.

@mlindes
Copy link
Contributor Author

mlindes commented Jul 31, 2017

Oh sorry, I spoke too soon. I didn't notice the circle.yml file is totally different in the current base.
Looks like the latest change to circle.yml was after the 1.3.5 tag. Maybe I can just pull that one file. Thoughts?

@danielnelson
Copy link
Contributor

I haven't tried that ever but when merge I will do a "Squash and merge" so as long as it does not introduce unwanted diffs it's fine.

@mlindes
Copy link
Contributor Author

mlindes commented Aug 9, 2017

ok. I rebased off master, and tests now pass. The rebase introduced a few commits... but there hasn't been a tag or version release to use as a rebase point since the latest changes to the circle.yml file.

@danielnelson
Copy link
Contributor

You will need to remove the unrelated commits during the rebase (use the -i flag).

@mlindes
Copy link
Contributor Author

mlindes commented Aug 10, 2017

Accidentally closed this while trying to get our fork in sync.
Please check out the new PR - #3112

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.

5 participants