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

Added sc-pipelines support #12

Merged
merged 34 commits into from Jun 14, 2018

Conversation

marcingrzejszczak
Copy link
Contributor

No description provided.

@pilloPl
Copy link
Member

pilloPl commented Jun 4, 2018

Thanks Marcin! Will take a look in the nearest future

@michal-michaluk
Copy link
Collaborator

michal-michaluk commented Jun 4, 2018

thx, cool, 3 points from me:

  1. tests you ware forced to ignore are my fault, they are already fixed in another PR Tech/move tests to inmemory db #11.
  2. why you dropped jsonb type in postgres, any issue with it? It is supported since postgresql 9.4 or even 9.3
  3. travis build is currently failing at assembly plugin execution (empty assembly), shouldn't that assembly be defined in factory/app-monolith instead of root module factory/, app is packed there in uber jar?

@marcingrzejszczak
Copy link
Contributor Author

marcingrzejszczak commented Jun 4, 2018

Ad. 2

It's failing on Cloud Foundry's postrgesql and I didn't have time to fix it

Ad. 3

Your build on Travis is running install (thus package) with skipping tests. No tests equals no stubs generated from rest docs. No stubs == broken build. I think you should fix your travis build

@michal-michaluk
Copy link
Collaborator

@marcingrzejszczak thx for info.
I will look on travis build, after addition of mvnw he picked some additional steps.
Could we have another liquidbase parameter value activated via context = ENV or JVM property for Cloud Foundry's postrgesql?

@marcingrzejszczak
Copy link
Contributor Author

Sure

@marcingrzejszczak
Copy link
Contributor Author

I fixed the conflicts and added limiting of connections for cloud (for free plan of postrgresql I ran out of connections). Also I still have to ignore one test cause it's not passing for me

@codecov-io
Copy link

codecov-io commented Jun 5, 2018

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master     #12   +/-   ##
========================================
  Coverage      97.4%   97.4%           
  Complexity      104     104           
========================================
  Files            23      23           
  Lines           231     231           
  Branches         29      29           
========================================
  Hits            225     225           
  Misses            4       4           
  Partials          2       2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0255f65...f5a6109. Read the comment docs.

@@ -41,9 +41,28 @@
</dependencies>

<properties>
<java.version>1.8</java.version>
<!-- [PIPELINE] -->
<distribution.management.release.id>artifactory-local</distribution.management.release.id>
Copy link
Member

Choose a reason for hiding this comment

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

localhost:8081? should be depend on hardcoded URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can override it and it happens via the pipeline

<changeSet author="jakubpilimon" id="2.postgres.json" dbms="postgresql">
<sql>CREATE CAST (character varying AS jsonb) WITH INOUT AS ASSIGNMENT</sql>
</changeSet>
<!--<changeSet author="jakubpilimon" id="2.postgres.json" dbms="postgresql">-->
Copy link
Member

Choose a reason for hiding this comment

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

commenting changes that might have already been run somewhere will cause problems :) I know you had problems with jsonb but you can at least add ignore on error for this changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to fix it

@@ -1,6 +1,7 @@
spring.main.banner-mode=off
spring.jpa.database=default
spring.jpa.database=postgresql
Copy link
Member

Choose a reason for hiding this comment

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

postgres for default profile? What about running locally?

import org.springframework.beans.factory.annotation.Autowired
import org.springframework.boot.test.context.SpringBootTest
import spock.lang.Specification

import static java.util.Collections.singletonList

// TODO: Unignore
Copy link
Member

Choose a reason for hiding this comment

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

PR that fixes this test is already merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work on my machine


def 'should work'() {
expect:
Awaitility.await().atMost(this.timeout, TimeUnit.SECONDS).untilAsserted({
Copy link
Member

Choose a reason for hiding this comment

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

Why you use awaitility when in Spock you have PollingConditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I copied it from a working JUnit test. Feel free to change it to whatever you want.

@Override
Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
if (bean instanceof ShortagesDao) {
return Mockito.mock(ShortagesDao)
Copy link
Member

Choose a reason for hiding this comment

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

hmm, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we want to mock out the access to the real database - spring-projects/spring-boot#13298

@pilloPl pilloPl merged commit e7e3393 into ddd-by-examples:master Jun 14, 2018
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