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

Fix Travis #178

Closed
wants to merge 1 commit into from
Closed

Fix Travis #178

wants to merge 1 commit into from

Conversation

bravo-kernel
Copy link

@bravo-kernel bravo-kernel commented Jul 5, 2019

This will be done step-by-step, all commits will be rebased when done.

Noticeable changes:

  • uses postgresql 9.4 (instead of 9.3)
  • adds a build.sh script for node >=10 (as it requires a specific OpenSSL version)
  • total runtime has increased significantly (caused by adding node12 + build.sh)

@bravo-kernel
Copy link
Author

bravo-kernel commented Jul 5, 2019

I see the pattern now.... it's the Node 10 + Postgres Native builds that are failing. Seems to be caused by somewhat outdated Travis machine versions using an odd OpenSSL version as described here, here and here where I think it would be best to mimic the PR in the second link.

Mental notes:

  • for Node.js equal or greater to version 10.16.0 you need to have at least OpenSSL 1.1.1 installed.

@overlookmotel
Copy link
Owner

Thanks loads for tackling this @bravo-kernel!

@overlookmotel
Copy link
Owner

overlookmotel commented Jul 5, 2019

By the way, it would be great to also run Travis on Node v12, but when I tried that it failed for all dialects.

I'm not asking you to do that! But once you have Node v10 working, maybe worthwhile seeing if it runs successfully on Node v12 too.

@bravo-kernel
Copy link
Author

bravo-kernel commented Jul 5, 2019

Not tackled by far yet but let's see where we end up. This has been brought up in the other repo where I found the workaround as well but would you consider moving this to Azure Pipelines? I have moved one of my repos to it about a month ago and must say that I am very impressed. Far more up-to-date and the various stages could come in handy.

Stage 1 would be building the Artifact (sequelize-hierarchy), then re-use that artifact in all following stages using a similar build matrix. Would visually look something like this:

image

@overlookmotel
Copy link
Owner

Azure Pipelines does look nice, and it'd be good to run tests on Windows too which Travis doesn't offer.

However, I'm using Travis as CI for all my projects, so would prefer to keep consistent. Aside from the issue with this specific repo, I've had no trouble with Travis at all and know how it works, so switching to something else would involve a learning curve which I don't think I have time for at the moment. Sorry to be negative, just being realistic about what I can handle at the moment.

@coveralls
Copy link

coveralls commented Jul 5, 2019

Coverage Status

Coverage remained the same at 90.374% when pulling 56f2e47 on alt3:travis into b87c8c2 on overlookmotel:master.

@bravo-kernel
Copy link
Author

bravo-kernel commented Jul 5, 2019

Totally understandable not moving to Azure.

Also, now that I've looked more closely at the issue I think it is a shame Travis does not seem to support artifacts out-of-the box (like Azure does) because I think this is where all (current and possibly future) problems are coming from:

  • we now MUST build the the (sequelize-hierarchy) extension on each platform
  • we would however only like to test the workings test/suite of the extension itself (and not the build-process itself), correct?

In other words, if we could separate the build from the test-matrix we end up in the best possible situation. Do you agree?

@bravo-kernel
Copy link
Author

Travis has documented examples of stages using artifacts on AWS S3 Storage, might be interesting:

@bravo-kernel
Copy link
Author

bravo-kernel commented Jul 5, 2019

@overlookmotel Looks like we are very close to a winner 💃. I've updated my initial comment with the noticeable changes.

The key was in changing the order; the databases have to be created before running the node >=10 build.sh script as that seems to alter the PATH in a funky way 💡

ps1: the last set of failing builds seem unrelated and caused by PPA, looks like they are updating their repo but unsure.
ps2: let's forget about AWS for now even though it would decrease the total time significantly IMO

@bravo-kernel
Copy link
Author

If you could re-trigger this build it would be nice, looks like PPA is back in business

@overlookmotel
Copy link
Owner

I've restarted the build. Let's see what happens...

@bravo-kernel
Copy link
Author

I say Travis is dodgy as ****. Those two failed builds differ from the previous fails and look like they just halted. Could you do one more re-trigger?

@overlookmotel
Copy link
Owner

Yes, it is annoying that Travis flunks from time to time. I feel like it's happening more often over past few months.

I restarted the build (restarted all the sub-builds for all dialects and versions, not just the ones which failed last time). Let's see if it passes this time...

@overlookmotel
Copy link
Owner

@bravo-kernel All passed! Amazing job.

I'll look through your changes and get this merged in next few days.

@bravo-kernel
Copy link
Author

bravo-kernel commented Jul 7, 2019

You're welcome, hopefully this will make judging future PRs a bit more easy for you. If you ignore the changes I made to the order/sorting you will see not much has changed.

The key was in changing the order; the databases have to be created before running the node >=10 build.sh script as that seems to alter the PATH in a funky way

@bravo-kernel bravo-kernel changed the title [Do not merge] Fix Travis Fix Travis Jul 7, 2019
@bravo-kernel
Copy link
Author

@overlookmotel any chance we can get this merged so we can start updating dependencies etc?

@bravo-kernel
Copy link
Author

@overlookmotel alive?

- "8"
- "10"
- "12"

Choose a reason for hiding this comment

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

14 too?

Copy link
Author

Choose a reason for hiding this comment

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

I would say both 14 and 15.

@overlookmotel
Copy link
Owner

@bravo-kernel I'm sorry I have treated you abysmally. You did a lot of work, and I've never got around to merging this PR.

Situation is that I no longer use this package myself, and my day job has got more and more demanding over past couple of years, so I have found myself very starved of time. It's been hard to prioritize this repo, as is doesn't feed into my own projects any more.

I have finally bowed to the inevitable and am seeking a new maintainer (#234). I imagine your own interests may have moved on elsewhere too by now, so I doubt you're interested, but just wanted to at least apologise for wasting your time.

@bravo-kernel
Copy link
Author

No need to apologize mr. @overlookmotel, you have explained yourself in the past.

Why not simply archive the repo? If people need it, they will either create a new fork or request ownership.

I personally put this image on top of the readme for my archived repos (before archiving).

abandoned-repo

This pull request was closed.
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.

4 participants