Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: runner use new image #3468

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

bodinsamuel
Copy link
Collaborator

@bodinsamuel bodinsamuel commented Feb 6, 2025

Changes

Contributes to https://linear.app/nango/issue/NAN-605/use-image-for-runner

@bodinsamuel bodinsamuel self-assigned this Feb 6, 2025
@bodinsamuel bodinsamuel marked this pull request as ready for review February 6, 2025 11:38
@bodinsamuel bodinsamuel requested a review from a team February 6, 2025 11:38
Copy link

linear bot commented Feb 6, 2025

Copy link
Contributor

@nalanj nalanj left a comment

Choose a reason for hiding this comment

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

Looks good. Definitely think Thomas should review too.

@nalanj nalanj requested a review from TBonnin February 6, 2025 13:15
Copy link
Collaborator

@TBonnin TBonnin left a comment

Choose a reason for hiding this comment

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

should we also remove building the older image in build-images.yaml?

plan: getPlan(node),
...(node.image.includes('-runner')
? {}
: { envSpecificDetails: { dockerCommand: 'node packages/runner/dist/app.js 80 dockerized-runner' } })
},
envVars: [
{ key: 'PORT', value: '80' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could have a port variable and use it here and in the dockerCommand so they are always in sync

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point

@bodinsamuel
Copy link
Collaborator Author

should we also remove building the older image in build-images.yaml?

Yes I'm just keeping it until the next successful deploy just in case I need to revert

@bodinsamuel bodinsamuel enabled auto-merge (squash) February 7, 2025 09:35
@bodinsamuel bodinsamuel merged commit bbd3721 into master Feb 7, 2025
17 checks passed
@bodinsamuel bodinsamuel deleted the sam/25_02_06/feat/runner-new-docker-image branch February 7, 2025 09:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants