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

update ci versions #65

Merged
merged 1 commit into from
Dec 9, 2021
Merged

update ci versions #65

merged 1 commit into from
Dec 9, 2021

Conversation

BCerki
Copy link
Collaborator

@BCerki BCerki commented Dec 8, 2021

This PR:

wenzowski
wenzowski previously approved these changes Dec 8, 2021
Copy link

@wenzowski wenzowski left a comment

Choose a reason for hiding this comment

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

Crucial!

@@ -114,5 +114,5 @@
}
]
},
"generated_at": "2021-12-08T00:06:35Z"
"generated_at": "2021-12-08T19:38:46Z"

Choose a reason for hiding this comment

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

No need to include this in the patch, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wenzowski, is there a way to un-include things like this without changing the commits and un-approving the PR?

Choose a reason for hiding this comment

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

I believe our approval settings are currently to dismiss stale reviews every time there's a change in the commit manifest of the PR

@@ -12,7 +12,7 @@ jobs:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
with:
node-version: '10'
node-version: '16'

Choose a reason for hiding this comment

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

This looks like continuation of #56 ...so I'd call the fact that we were able to bump the node version there and miss the CI configuration tech debt. Let's not hold back this PR but let's figure out how to ensure when we bump node versions in the future we get everything in one go–perhaps we should be looking at how other projects use a .tool-versions file as a source of truth that gets consumed by other systems, like docker, ci, npm, etc.

Copy link

@wenzowski wenzowski left a comment

Choose a reason for hiding this comment

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

Let's ship & record tech debt as ticket

@BCerki BCerki merged commit 7331ab8 into main Dec 9, 2021
@BCerki BCerki deleted the 64-update-ci-versions branch December 9, 2021 18:07
@BCerki
Copy link
Collaborator Author

BCerki commented Dec 9, 2021

Let's ship & record tech debt as ticket

Here: https://github.com/button-inc/digital_marketplace/issues/67

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

2 participants