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 dependencies (containerd v2) #290

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Jun 24, 2024

What this PR does / why we need it:

Hi folks.

This PR updates dependencies, with a specific focus on containerd (v2).

It also bumps the go version to align with ^, upgrades github actions and urfave (v2).

Please NOTE that currently this is swapping out github.com/data-accelerator/zdfs to github.com/apostasie/zdfs and should of course NOT be merged as-is. Evidently, as soon as the PR on zdfs gets through I will update this to make it ready (data-accelerator/zdfs#3).

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
    • No new test
  • Does this change require a documentation update?
    • Golang version requirement has been updated
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
    • I believe so. Although there is nothing that should break API compatibility, this is a major change in dependencies.
  • Do all new files have an appropriate license header?
    • n.a.

Let me know your thoughts.

Thanks a lot.

@liulanzheng liulanzheng added the ok-to-test Pull request is ok to run ci test label Jun 26, 2024
@apostasie
Copy link
Contributor Author

@liulanzheng not quite sure why the CI would use go 1.19 (as I did upgrade to 1.22)?
Is the env GO_VERSION set outside of the action files?

Thanks.

@apostasie
Copy link
Contributor Author

Rebased against latest main.

e2e still failing as they rely on go 1.19 apparently.

@liulanzheng
Copy link
Member

@yuchen0cc It seems that the CI was using the script from the main branch instead of the script modified in the PR. How should this situation be handled?

@liulanzheng
Copy link
Member

@apostasie After confirming with @yuchen0cc, to prevent potential attacks from CI, the CI runs the code from the target branch. Therefore, we have to merge the code first before running the new CI. Additionally, could we merge the zdfs first before merging this branch?

@apostasie
Copy link
Contributor Author

@apostasie After confirming with @yuchen0cc, to prevent potential attacks from CI, the CI runs the code from the target branch. Therefore, we have to merge the code first before running the new CI. Additionally, could we merge the zdfs first before merging this branch?

Thanks @liulanzheng !

For sure - we do need the zdfs PR merged first.
Akihiro just pinged someone over there, so, let's give it some time for them to react?

If you want, I can ping you back here once it is done and this one is updated to reflect that.

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
@apostasie
Copy link
Contributor Author

Hey @liulanzheng

zdfs PR got merged last night.

I just updated this here so that we are back on zdfs main instead of my fork (thanks to @AkihiroSuda help).

I guess this here is ready for review (or merge on a feature branch?)

Let me know if you need anything else here from me.

Thanks a lot!

Copy link
Member

@liulanzheng liulanzheng left a comment

Choose a reason for hiding this comment

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

LGTM. Only involves dependency changes. It is recommended to merge first and then check the e2e results.

@liulanzheng liulanzheng merged commit 3c4dd55 into containerd:main Jul 12, 2024
8 of 9 checks passed
@liulanzheng
Copy link
Member

All CI passed

@apostasie
Copy link
Contributor Author

Thanks a lot @liulanzheng !

Appreciate the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Pull request is ok to run ci test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants