-
Notifications
You must be signed in to change notification settings - Fork 1
SERVER-216 #76
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
SERVER-216 #76
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
=======================================
Coverage 76.15% 76.15%
=======================================
Files 13 13
Lines 1086 1086
=======================================
Hits 827 827
Misses 190 190
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
.github/docker/entrypoint.sh
Outdated
| install_deps_el10 | ||
| elif [ "$ENV_DISTRO" = "amzn2023" ]; then | ||
| echo "installing dependencies for Amazon 2023" | ||
| install_deps_redhat-amzn2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be "install_deps_amzn2023"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good but some of these items should be addressed (some are nits). Also it could use yamlint and shellchk (plus formatting). Trunk is a good tool for this if you haven't used it. I added the baseline trunk config as a PR in case you want to take it but have commented out the commit rules until you are adopting those.
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| branches: [ main, SERVER-216, SERVER-434 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to run the build only on merge to main? Wouldn't we need to do something on PR also (either just build or build and test)
in either case should remove the branches
| branches: [ main, SERVER-216, SERVER-434 ] | |
| branches: [ main ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the packaging CI workflow, I imagine we want to only build and push artifacts for the builds that are ready to go to QE.
So either merge to main, or tag creation.
The projects testing workflow is separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it build on this branch, and on tag creation.
Lets talk about this with QE, anyway removing the SERVER-216 should be the last commit here before we merge
| runs-on: ${{ matrix.host }} | ||
| project: database | ||
| build-id: ${{ github.run_number }} | ||
| build-script: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to && these together they can just be an inline shell script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not continue if one of the commands failed, but set -e would do the same thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set -e runs by default (it is the default when shell part of a github action so the script mimics that behaviour
.github/docker/entrypoint.sh
Outdated
|
|
||
| if [ "$INSTALL" = false ] && [ "$BUILD_INTERNAL" = false ] && [ "$BUILD_CONTAINERS" = false ] && [ "$EXECUTE_BUILD" = false ]; | ||
| then | ||
| echo """Error: Options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the triple quote convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a real convention.
| # artifact-name: signed-artifacts # optional, defaults to signed-artifacts | ||
| # retention-days: 7 # optional, defaults to 7 | ||
| # dry-run: false # optional, for future compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove these commented out lines
arrowplum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I see it is not using the bundle workflow. A bundle will have to be created in order to release or promote the artifacts. https://github.com/aerospike/shared-workflows/blob/main/.github/workflows/reusable_create-release-bundle.yaml
| runs-on: ${{ matrix.host }} | ||
| project: database | ||
| build-id: ${{ github.run_number }} | ||
| build-script: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set -e runs by default (it is the default when shell part of a github action so the script mimics that behaviour
| workflow_dispatch: | ||
| push: | ||
| branches: [ main, SERVER-216, SERVER-434 ] | ||
| branches: [ SERVER-216 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't test merges to main but shouldn't this run for PRs not just for testing but so PR commits get tested?
48addf9 to
e7faa50
Compare
arrowplum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
arrowplum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved but I think the trigger is still something wrong (should be able to trigger on a pull request)
checking to see if it is the fault of the pinned commit |
|
Found the culprit. A fix there would be a one liner but investigating I started with upgrading to 2.0.1 (i hadn't noticed the ref issue yet). It is fairly straightforward to upgrade so added a pr for both (into this branch) #80 |
| branches: | ||
| - main | ||
| schedule: | ||
| - cron: '0 0 * * *' # run at Midnight UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing changes we shouldn't produce a new build every day should we? Also I think you don't want to get rid of builds on PR (even if it doesn't do the deploy or bundle step)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see @kportertx's comment here
https://aerospike.atlassian.net/browse/SERVER-216?focusedCommentId=169529
If we cant do this I think we will need to update to an alternate deploy target and self-hosted runners, but I dont think that is in the scope of this release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @arrowplum, will need to write up something. @arrowplum suggested (which you'd also suggested something similar) to build and test all commits but to only upload to jfrog commits to PRs. To keep the rules simple across projects, don't worry with "PRs that target main" since different projects have various branches used for release processes. The assumption is that a very small subset of PRs don't target a release branch.
Will also update the PRD with this info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kportertx OK so to clarify, we would like to rpm and dev packages on:
- branch prs:
- dev/*
- hotfix/*
- tags
Delivering these builds to QE I assumed will go through the packaging system - although if we want to keep the old system as well thats a possibility.
If we need to deliver packaged builds to QE on every commit we will need another place to store them (flat file bucket comes to mind)
|
Moving common/ code to https://github.com/aerospike/server-packaging-common/ |
pkg/Makefile
Outdated
| # Package variables | ||
| NAME = "asconfig" | ||
| VERSION = $(shell git describe --tags --always) | ||
| BUILD_DISTRO ?= $(shell .github/packaging/common/os_version.sh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be:
BUILD_DISTRO ?=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be resolved now
pvinh-spike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no asconfig target in top Makefile so the following need to be changed:
.PHONY: all
all: $(ACONFIG_BIN) deb rpm tar
.PHONY: deb
deb: $(ACONFIG_BIN)
.PHONY: rpm
rpm: $(ACONFIG_BIN)
.PHONY: tar
tar: $(ACONFIG_BIN)
pvinh-spike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to have usages/descriptions in scripts. It will make maintenance them easier.
pvinh-spike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming warning from github-advanced-security[bot] reviewed and determined no changes needed.
| - el10 | ||
| - amzn2023 | ||
| - debian12 | ||
| #- debian13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this in the walkthrough but noting it for later. re-enable this when possible.
dwelch-spike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM left a comment as a reminder of something that came up in the walkthrough but this looks good as is to me 👍
| #!/usr/bin/env bash | ||
| set -xeuo pipefail | ||
| DISTRO="$1" | ||
| env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for debugging? Do we know it this may dump secrets to the log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kportertx I think the secrets are redacted by value in the logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it is for debugging
412cf2d to
211d847
Compare
Built packages with links to JFrog are available in the
Actionstab aboveIntegration tests are here.
Test cases download the current build from JFrog and install it in a fresh container, then execute it with the --help argument
You need a JFrog username and token to execute the test cases, an example is in the README.sh