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

Improve container image tags and timestamp #2757

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Improve container image tags and timestamp #2757

merged 1 commit into from
Mar 15, 2024

Conversation

Qusic
Copy link
Contributor

@Qusic Qusic commented Nov 12, 2023

resolves #2477

This implementation builds for all platforms in one workflow step. I think it makes sense since all binaries are already ready at that time.
Another option is to keep using strategy matrix but we will have to upload all image manifest archives to workflow artifacts and merge them in another workflow job as demonstrated here, which seems not worth the trouble in our case. But I am open for discussion.

Currently for each release, these tags will be added for different version ranges and variations. Take v5.11.0 for example:

  • v5.11.0 (no suffix for std variation)
  • v5.11
  • v5.11.0-extra
  • v5.11-extra

Besides, if it's not marked as a prerelease, these additional tags will be added:

  • v5
  • latest
  • v5-extra
  • latest-extra

I also updated the timestamp to release creation time instead of 0. This should keep the reproducibility and make it more user-friendly to have a reasonable created time.

As for the Containerfile change, I think it's important to copy those files from an updated image, especially certificates store.

One successful test run for building job can be found here. (Pushing job failed due to permission issue)

@xiaokangwang xiaokangwang added the Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval label Nov 18, 2023
Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

I have made 2 suggests to make sure this merge request does not break design goals of the current system. I understand these changes are not easy, and it is the reason they are not implemented earlier in the first place.

Reproducibility are there to let users know the software are build from source as is without backdoor. This not only protects users from supply chain attacks from bad build machines, but also protect developers as it make sure there is no way to add a backdoor making it less attractive to pressure developers into adding backdoor into their software.

The distinction between unstable and stable release help developers to generate beta and unstable releases for user testing without the fear of major breakage of unattended systems. This significantly decrease the need to make sure a software is bug free before making an unstable release. As with all open source projects, V2Ray have limited developer resources and could not make extensive test of new features before they are released and particular depend on user testing of unstable releases to discover bugs. For this reason it is important to be able to create unstable releases that are not released to unattended systems.

release/container/Containerfile Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@Qusic Qusic requested a review from xiaokangwang November 22, 2023 13:51
@xiaokangwang
Copy link
Contributor

I think it no longer support the case where a stable release is created directly, without go through unstable phase?

@Qusic
Copy link
Contributor Author

Qusic commented Nov 24, 2023

I think it no longer support the case where a stable release is created directly, without go through unstable phase?

I tested in my fork. When promoting a prerelease to stable release by editing it, a released action will be triggered. This behavior is also documented here.

Or are you referring to those github.event.action == 'prereleased' checks I added? This is just to keep the original workflow behavior as it was only triggered by prerelease action before. So it seems to me that we never supported a stable release directly created without being a prerelease first. Please elaborate more if I misunderstand anything.

@xiaokangwang
Copy link
Contributor

Or are you referring to those github.event.action == 'prereleased' checks I added? This is just to keep the original workflow behavior as it was only triggered by prerelease action before. So it seems to me that we never supported a stable release directly created without being a prerelease first. Please elaborate more if I misunderstand anything.

This will happen if a release have already been published, and a critical bug is discovered and then fixed. Such an emergency fix will not go through prerelease. This is uncommon but may happen.

@Qusic
Copy link
Contributor Author

Qusic commented Dec 13, 2023

Or are you referring to those github.event.action == 'prereleased' checks I added? This is just to keep the original workflow behavior as it was only triggered by prerelease action before. So it seems to me that we never supported a stable release directly created without being a prerelease first. Please elaborate more if I misunderstand anything.

This will happen if a release have already been published, and a critical bug is discovered and then fixed. Such an emergency fix will not go through prerelease. This is uncommon but may happen.

Added released to triggered action list. This should work in all cases where new releases are created or the types of existing releases are changed. However, the overwrite argument is required for duplicate uploads to succeed according to the implementation.

@Qusic
Copy link
Contributor Author

Qusic commented Mar 12, 2024

@xiaokangwang friendly ping in case you missed it. please review the latest update.

@xiaokangwang
Copy link
Contributor

I didn't see any security issue with this pull request. I will merge it now and see if it work in practice. If it does create a problem, I will resolve it myself with https://github.com/estesp/manifest-tool.

@xiaokangwang xiaokangwang merged commit 887280e into v2fly:master Mar 15, 2024
1 check passed
@0794-8819-0610-5047
Copy link

resolves #2477

This implementation builds for all platforms in one workflow step. I think it makes sense since all binaries are already ready at that time. Another option is to keep using strategy matrix but we will have to upload all image manifest archives to workflow artifacts and merge them in another workflow job as demonstrated here, which seems not worth the trouble in our case. But I am open for discussion.

Currently for each release, these tags will be added for different version ranges and variations. Take v5.11.0 for example:

  • v5.11.0 (no suffix for std variation)
  • v5.11
  • v5.11.0-extra
  • v5.11-extra

Besides, if it's not marked as a prerelease, these additional tags will be added:

  • v5
  • latest
  • v5-extra
  • latest-extra

I also updated the timestamp to release creation time instead of 0. This should keep the reproducibility and make it more user-friendly to have a reasonable created time.

As for the Containerfile change, I think it's important to copy those files from an updated image, especially certificates store.

One successful test run for building job can be found here. (Pushing job failed due to permission issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestions on container tags
3 participants