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

Make different artifacts.go included in the release #2554

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zeroalphat
Copy link
Contributor

Make different artifacts.go included in the release for different environments.

@zeroalphat zeroalphat self-assigned this May 1, 2024
@zeroalphat zeroalphat marked this pull request as ready for review May 2, 2024 00:40
Copy link
Contributor

@masa213f masa213f left a comment

Choose a reason for hiding this comment

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

As I commented elsewhere, please consider whether we can stop using the build tag.

Comment on lines +462 to +473
# triggered weekdays at 15:00 UTC(00:00 UTC+9) on the main branch,
# check if a new artefact has been released.
daily-check-new-artifacts:
triggers:
- schedule:
cron: "0 15 * * 1-5"
filters:
branches:
only:
- main
jobs:
- check-new-artifacts
Copy link
Contributor

Choose a reason for hiding this comment

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

This workflow could be done with the daily workflow. How about merging them?
If we separate this workflow, we need to check the two links (CI result) in the daily confirmation.

Comment on lines +271 to +272
diffs=$(git diff --name-only main)
if [ "$diffs" = "" ]; then printf "%s\n" "$diffs"; exit 0; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

The comparison seems inverted.

Suggested change
diffs=$(git diff --name-only main)
if [ "$diffs" = "" ]; then printf "%s\n" "$diffs"; exit 0; fi
diffs=$(git diff --name-only main)
if [ "$diffs" != "" ]; then printf "%s\n" "$diffs"; exit 0; fi

And it could be simplified.

Suggested change
diffs=$(git diff --name-only main)
if [ "$diffs" = "" ]; then printf "%s\n" "$diffs"; exit 0; fi
git --no-pager diff --name-only main

I suppose that this run step is just for information.
We don't want to stop the sequence even if there were differences, right?

git diff without --exit-code option returns zero even if there were differences.
You can directly use this command.
In case you didn't know, if git diff returned non-zero, the assignment diffs=$(git diff ...) also returns non-zero and fails.

--no-pager may not be necessary.

- deploy_github:
name: "build release with release prefix"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: "build release with release prefix"
name: "deploy release with release prefix"

Comment on lines 4 to 5
`generate-artifacts` is a command-line tool to generate `artifacts.go`
which is a collection of latest components. This is used while
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`generate-artifacts` is a command-line tool to generate `artifacts.go`
which is a collection of latest components. This is used while
`generate-artifacts` is a command-line tool to generate `artifacts.go`,
which is a collection of the latest components. This tool is used in development.
It is also used in CI to check differences between the latest release and the versions in use.

Comment on lines -55 to +56
c.check = github.GetLatestPublishedTag
github.SetTagPrefix("test-")
c.check = github.GetLatestReleaseTag
Copy link
Contributor

@morimoto-cybozu morimoto-cybozu May 14, 2024

Choose a reason for hiding this comment

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

Do we need to change this?

This change should require much more changes in CI workflows, tools, and documents.
Please let me investigate more.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least the following should be updated:

  • docs/cicd.md
    • The steps of "deploy for the staging data center" should be updated to include the following.
      • Remove the "pre-release" label from the target test- release.
    • It is still necessary to create a release- tag. This may be done automatically for a released test- release.
    • It currently lacks a description for the dev data center, i.e., stage1.
  • download-neco-deb
    • This should prepare the same neco.deb that is used in the staging data center. We need to use the same algorithm with neco-updater.
    • I'm not sure whether we should update this just now or delay the update until we release this PR into stage0. Probably the former because this command is used in the reproduced staging environment.
  • find-installed-release
    • This should return the same "datetime-rev" release name that is used in the staging data center.
    • I'm not sure whether we should update this just now or delay the update. Probably the latter because this command is used to reproduce the staging environment.
    • The caller should be updated too.

I may have overlooked something.


type args struct {
repo name.Repository
release bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used because getLatestImage() no longer receives a release parameter.

Comment on lines +68 to +96
{
name: "can set ignored versions that is no-release version",
args: args{
repo: testRepository,
release: false,
ignoredVersions: []string{"100.0.0"},
},
tagsList: []byte(`{"tags":["0.1.0","0.2.0", "1.0.0"]}`),
img: &neco.ContainerImage{
Name: imageName(testRepository),
Repository: testRepository.String(),
Tag: "1.0.0",
Private: false,
},
},
{
name: "should get latest image with release",
args: args{
repo: testRepository,
release: true,
},
tagsList: []byte(`{"tags":["0.1.0","0.2.0", "1.0.0"]}`),
img: &neco.ContainerImage{
Name: imageName(testRepository),
Repository: testRepository.String(),
Tag: "1.0.0",
Private: false,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

These two cases are the same.

@@ -4,24 +4,22 @@ Artifacts
[Artifacts](../artifacts.go) is the collection of components tested in [dctest](../dctest/), which consists of container images, debian package, and Flatcar image.

There are two artifacts files. When generating neco binaries and the neco deb package, `artifacts.go` is used
by default. `artifacts_release.go` is used only when `release` build tag is given.
by default. `artifacts_prod.go` is used only when `release` build tag is given.

- [artifacts.go](../artifacts.go)

This file describes artifacts to be tested in development.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This file describes artifacts to be tested in development.
This file describes artifacts to be tested in development and staging.


This file is for CI. The CI job updates this file by using
`generate-artifacts --release`.
This file describes artifacts to be tested in production.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This file describes artifacts to be tested in production.
This file describes artifacts to be used in production.

Comment on lines 20 to 21
The developers are PROHIBITED to modify this file. This is to prevent
merge conflicts in CI flow.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is prohibited so strictly now.

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