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

Netlify integration #30

Closed
wants to merge 27 commits into from
Closed

Netlify integration #30

wants to merge 27 commits into from

Conversation

denar90
Copy link
Collaborator

@denar90 denar90 commented Feb 16, 2020

Add support for integration with Netlify Preview Site deploy

User changes stuff for the site and creates PR -> Netlify deploy Preview site -> Action do the check against Preview site and show send notification via github/slack

image

Example I tested stuff


Todo:

  • docs
  • race conditions, Netlify can deploy longer then action starts to check, we need to wait for the Netlify to finish and then run action
  • add/improve options

P.S. I'll squash commits after final round of feedback :)

@denar90 denar90 changed the base branch from master to slack-integration February 16, 2020 00:55
@denar90
Copy link
Collaborator Author

denar90 commented Feb 23, 2020

@denar90 denar90 marked this pull request as ready for review February 29, 2020 21:27
@denar90 denar90 changed the base branch from slack-integration to next February 29, 2020 21:28
@denar90
Copy link
Collaborator Author

denar90 commented Feb 29, 2020

@exterkamp can I ask you to take a look? 👀

Copy link
Collaborator

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

These are cool changes, overall they seem good. I have some questions though.

Can I provide urls that go to a single netlify site, or multiple sites? Can I mix netlify sites and regular sites?

Why do we need a netlifySite param? Seems like we could look at the domains of the URLs and tell that they need to be adjusted. However, then how do you mix preview-deploy URLs and normal netlify hosted URLs? Is this something we want to support.

I'm also curious if this integrates with the netlify github action? Is there a way to show how you would use both in the recipe?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
core.startGroup('Action config')
console.log('Input args:', input)
core.endGroup() // Action config

/*******************************WARMING UP***********************************/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should be called "Warming up" or something like "Waiting for preconditions"?

src/index.js Outdated
core.startGroup('Action config')
console.log('Input args:', input)
core.endGroup() // Action config

/*******************************WARMING UP***********************************/
if (input.netlifySite) {
core.startGroup('Warming up')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this would be more descriptive with a group like "Waiting for Netlify site"

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
}
console.log('Waiting for build to be done')
await sleep()
console.log(`Retry ping Netlify`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These retry messages seem excessive, since there will be a "Pinging..." log as soon as the function is called.

src/index.js Outdated
console.log('Resolve Netlify site error', e)
return Promise.resolve(1)
}
console.log('Waiting for build to be done')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like these waiting msg's could be DRY'd up?

Copy link
Collaborator Author

@denar90 denar90 Mar 1, 2020

Choose a reason for hiding this comment

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

I've updated, hope I got your point :)

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
const retryNumber = 5
let runs = 0
const sleep = async () => {
return new Promise(r => setTimeout(r, 60000))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a good idea to pull out the 60000 ms out so that it can be modified easier/used to dynamically output how long the system is waiting in console logs.

Then maybe modify the logs to say Waiting additional ${seconds} seconds for Netlify build to be done....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've been thinking about it for the next iteration after getting feedback, I didn't wanna overwhelm action settings :)

denar90 and others added 3 commits March 1, 2020 23:00
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
denar90 and others added 2 commits March 1, 2020 23:01
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
@denar90
Copy link
Collaborator Author

denar90 commented Mar 1, 2020

Can I provide urls that go to a single netlify site, or multiple sites? Can I mix netlify sites and regular sites?

What's the use case, using Netlify urls, and some other urls in scope of one repository? My intention was the next, if user has deployed netlify site from github, then in scope this repo he/she can use action for that urls.


Why do we need a netlifySite param?

Sorry that's my bad, I'll update doc. So, Netlify site can have custom domain, and in this case we can't compose preview url properly. Thank you for bringing it up, It's a really good point because I haven't tested this one yet 👍

image

However, then how do you mix preview-deploy URLs and normal netlify hosted URLs? Is this something we want to support.

I think it's good to have support of real site and preview. Preview is composed based on PR info -

if (ref) {
origin = `https://${ref}--${netlifySite}`
}
if (isNumber(Number.parseInt(ref))) {
origin = `https://deploy-preview-${ref}--${netlifySite}`
}

If it's a master, aka action config for push event, url will be like https://master--practical-allen-be16f3.netlify.com/, but if it's PR, url will look like https://deploy-preview-1--practical-allen-be16f3.netlify.com, where number is a number of PR in repo.


I'm also curious if this integrates with the netlify github action? Is there a way to show how you would use both in the recipe?

Cool, need to play with it. Will get back with info :)

denar90 and others added 3 commits March 2, 2020 00:39
Co-Authored-By: Shane Exterkamp <shaneexterkamp5@gmail.com>
@denar90
Copy link
Collaborator Author

denar90 commented Mar 1, 2020

@exterkamp thank you so much for the review and questions 😍 I hope to find time to during the week to add more comments to code and also to clarify all statements related to the confusion between different urls to run. Feel free to ask more :)

This was referenced Mar 6, 2020
@alekseykulikov alekseykulikov deleted the netlify branch March 19, 2020 19:22
@alekseykulikov
Copy link
Member

@denar90 and I agreed, instead of a custom integration, provide an official Netlify recipe, using one of the community actions: https://github.com/marketplace?type=actions&query=netlify

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.

3 participants