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

_ci_: update snapcraft and release flow logic #8994

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

ianconsolata
Copy link
Contributor

Related Issues

https://app.circleci.com/pipelines/github/filecoin-project/lotus/21961/workflows/3bb820f2-4f7a-43d9-880f-7fe4dfecdc72/jobs/513420
Snap builds were previously failing due to an authentication issue. I think it was likely caused by outdated credentials, but snapcraft has also changed their auth method to use an env variable recently and deprecated the old value. So I set that new env var in CircleCI project settings, meaning we should be able to remove this section of CI where we write the old auth file.

I also tweaked the jobs for snapcraft and dockerhub to do trial-runs on all release branches, but still only publish on release tags. While it wouldn't have caught this issue, since it was publish related, I think it's still good practice to add this as a small sanity check.

@ianconsolata ianconsolata requested a review from a team as a code owner July 7, 2022 23:04
@ianconsolata ianconsolata changed the title Update snapcraft release flow Update snapcraft and release flow logic Jul 10, 2022
@ianconsolata ianconsolata force-pushed the run-builds-on-release-branches branch from 5eae900 to 9daa1ee Compare July 11, 2022 17:10
@ianconsolata
Copy link
Contributor Author

I opted to make modified copies of scripts/publish-release and scripts/build-bundle because I wasn't sure where else those scripts might be used, and if it was safe to modify them. If they are not used anywhere else, I can modify this PR to edit those scripts in place instead.

@ianconsolata ianconsolata force-pushed the run-builds-on-release-branches branch from 887f30d to c7ce987 Compare July 19, 2022 00:21
@ianconsolata ianconsolata changed the title Update snapcraft and release flow logic _ci_: update snapcraft and release flow logic Jul 19, 2022
@ZenGround0 ZenGround0 self-requested a review July 19, 2022 02:14
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

This is well outside my area of expertise but looks ok

@@ -115,7 +119,8 @@ jobs:
command: |
git --no-pager diff go.mod go.sum
git --no-pager diff --quiet go.mod go.sum
build-all:

build-linux:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only builds linux binaries, there are separate build tasks for other target platforms. I think the all was supposed to mean it's building all lotus commands, but I found it very confusing. build-linux matches pattern used elsewhere better (i.e. build-macos)

parameters:
linux:
default: false
description: publish linux binaries?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the question mark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make the comment clearer, but the parameter is to indicate whether the linux binaries should be published or not. I usually use ? to indicate true/false things.

@@ -713,14 +741,19 @@ jobs:
at: "."
- packer_build:
template: tools/packer/lotus-snap.pkr.hcl
publish-dockerhub:

build-dockerhub:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change name? The old name matches description better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it doesn't always publish anymore, the publish is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this for now though. It was a minor change, and not really that important.

mkdir bundle
pushd bundle

BINARIES=(

Choose a reason for hiding this comment

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

This array is unused.

@@ -78,6 +78,7 @@ do
echo "Uploading ${RELEASE_FILE}..."

Choose a reason for hiding this comment

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

Is this file still used? Now we have publish-arch-release that seems to be similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I wasn't sure if it was referenced anywhere outside of the circleci file. I removed it for now.

@coryschwartz
Copy link

@ianconsolata we need to make the gen-check test pass as well.... This check actually re-generates the circleci config from template and checks for differences.

@ianconsolata ianconsolata force-pushed the run-builds-on-release-branches branch from c7ce987 to 8a41a6b Compare July 25, 2022 05:18
@ianconsolata ianconsolata merged commit aa0aacc into master Jul 25, 2022
@ianconsolata ianconsolata deleted the run-builds-on-release-branches branch July 25, 2022 17:28
@rjan90 rjan90 mentioned this pull request Jul 26, 2022
5 tasks
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