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.

Sign up for GitHub

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

Jump to bottom

INT-536_remove_aws_config #34

Merged
merged 5 commits into from
Apr 26, 2018
Merged

INT-536_remove_aws_config #34

merged 5 commits into from
Apr 26, 2018

Conversation

fernau
Copy link
Contributor

@fernau fernau commented Apr 20, 2018 •
edited
Loading

Description

Hazelcast configuration is no longer updated in the default recipe when building docker images.

Jira

https://issues.sonatype.org/browse/INT-536

Screencast

https://youtu.be/-qaquP14mJw

@fernau fernau force-pushed the INT-536_remove_aws_config branch 5 times, most recently from 34e9204 to c3e1fa6 Compare April 20, 2018 15:40
collinpeters
collinpeters approved these changes Apr 20, 2018
View reviewed changes
Copy link
Member

@collinpeters collinpeters left a comment

Choose a reason for hiding this comment

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

+1 LGTM but I'll admin I don't know chef well so feel free to consider this a +0 😀

whyjustin
whyjustin reviewed Apr 20, 2018
View reviewed changes
recipes/hazelcast.rb Outdated
@@ -0,0 +1,14 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be called hazelcast but rather aws-hazelcast or something indicating it is for aws?

Copy link
Contributor Author

@fernau fernau Apr 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this is not strictly AWS specific (also supports static, multicast,..). so I renamed it to ha-hazelcast.
we might add HA for redhat or cubernetes,... c4917c8

recipes/hazelcast.rb Outdated
hazelcast_xml = node['nexus_repository_manager']['nexus_home']['path'] + '/etc/fabric/hazelcast.xml'

template hazelcast_xml do
source 'hazelcast.xml.erb'
Copy link
Member

Choose a reason for hiding this comment

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

Likewise this file should indicate it's aws specific.

@fernau fernau force-pushed the INT-536_remove_aws_config branch 18 times, most recently from a0a0346 to 7017fc8 Compare April 23, 2018 14:30
@fernau fernau force-pushed the INT-536_remove_aws_config branch 3 times, most recently from 381cccd to c4917c8 Compare April 23, 2018 16:02
@fernau fernau force-pushed the INT-536_remove_aws_config branch 3 times, most recently from a816775 to 2c9342c Compare April 23, 2018 19:17
@fernau fernau force-pushed the INT-536_remove_aws_config branch 2 times, most recently from bdd7728 to ef15abb Compare April 23, 2018 19:43
@fernau fernau force-pushed the INT-536_remove_aws_config branch 2 times, most recently from b5d902b to a0ce7fa Compare April 23, 2018 21:20
bigspotteddog
bigspotteddog approved these changes Apr 23, 2018
View reviewed changes
test/smoke/default/configure_test.rb
its('content') { should include '<password>nexus123</password>' }
end

context 'when multicast is enabled' do
Copy link
Contributor

@bigspotteddog bigspotteddog Apr 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like we dropped tests for some of these? They are still in the template.

  • multicast enabled
  • tcp-ip disabled
  • aws disabled
  • iam_role
  • region
  • tag key/value
  • hazelcast discovery disabled

Do we not need to test these?

+1 pending the answer to this question

Copy link
Contributor Author

@fernau fernau Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

these tests are removed only from this place because they no longer apply to the default configuration, but they are duplicated across all the other tests. for example (not exclusively):

multicast enabled is here: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/multicast_enabled/configure_test.rb#L13

tcp-ip disabled: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/multicast_enabled/configure_test.rb#L29

aws disabled: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/multicast_enabled/configure_test.rb#L25

iam_role: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/aws_enabled/configure_test.rb#L13

region: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/aws_enabled/configure_test.rb#L21

tag: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/aws_enabled/configure_test.rb#L25

discovery disabled: https://github.com/sonatype/chef-nexus-repository-manager/blob/master/test/smoke/aws_enabled/configure_test.rb#L41

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks!

@fernau fernau force-pushed the INT-536_remove_aws_config branch from a0ce7fa to 2efff6c Compare April 24, 2018 14:56
@fernau fernau force-pushed the INT-536_remove_aws_config branch from 2efff6c to abffc63 Compare April 24, 2018 14:57
@fernau fernau merged commit c2e3ff8 into master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigspotteddog bigspotteddog bigspotteddog approved these changes

@whyjustin whyjustin whyjustin left review comments

@collinpeters collinpeters collinpeters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fernau @whyjustin @bigspotteddog @collinpeters