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

feat: allow specifying nightly builds #151

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Oct 10, 2024

Warning

Needs testing!

The version field now accepts two additional fields:

  • A sha1 git hash of a main commit
  • The value "head" for the latest commit on main

This will allow users to put in nightly builds into their action.ymls, so that we can more easily allow testing with new versions of dagger.

@jedevc jedevc requested a review from gerhard October 10, 2024 15:18
@sagikazarmark
Copy link

/home/runner/work/_temp/8df93b16-410d-4bad-bb0e-b4276f155e09.sh: line 21: syntax error near unexpected token `else'

https://github.com/sagikazarmark/daggerverse/actions/runs/11281337948/job/31376343680

action.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@jpadams jpadams left a comment

Choose a reason for hiding this comment

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

@jedevc I tested this logic locally and it works well

# Determine the version to install
if [[ "$VERSION" == "latest" ]]; then
  # Get the latest version.
  #
  # Set the version back to an empty string. This allows the install
  # script to detect and install the latest version itself.
  VERSION=""
elif [[ "$VERSION" =~ ^v ]]; then
  # Get the specified version.
  #
  # No need to set anything.
  :
elif [[ "$VERSION" == "head" ]]; then
  # Get the latest nightly build from "main".
  #
  # Set the commit to "head".
  COMMIT="$VERSION"
  VERSION=""
elif [[ "$VERSION" =~ [a-z0-9]{40} ]]; then
  # Get the specified nightly build from "main".
  #
  # Set the commit to the specified commit sha.
  COMMIT="$VERSION"
  VERSION=""
else
  echo "Invalid version specified: $VERSION"
  exit 1
fi

TIL to use : as noop in bash

@jedevc jedevc force-pushed the allow-specifying-nightly-commit branch from 4f282f7 to 259c2cd Compare October 11, 2024 09:34
@jedevc jedevc requested a review from jpadams October 11, 2024 09:35
@sagikazarmark
Copy link

Run jedevc/dagger-for-github@259c2cdbc8915a3529d4f9c132208c597391104e
  with:
    verb: call
    module: github.com/sagikazarmark/daggerverse/apko/tests@refs/heads/main
    args:  all
    version: head
    dagger-flags: --progress plain
    workdir: .
    engine-stop: true
  env:
    GITHUB_TOKEN: ***

Invalid version specified:

action.yml Outdated
# If the dagger version is 'latest', set the version back to an empty
# string. This allows the install script to detect and install the latest
# version itself
VERSION=${{ inputs.version }}

Choose a reason for hiding this comment

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

Probably needs this line?

@jedevc jedevc force-pushed the allow-specifying-nightly-commit branch from 259c2cd to c524562 Compare October 11, 2024 16:31
@jedevc jedevc marked this pull request as draft October 11, 2024 16:31
@sagikazarmark
Copy link

@jedevc jedevc marked this pull request as ready for review October 14, 2024 16:12
@jedevc jedevc mentioned this pull request Oct 14, 2024
@jedevc
Copy link
Member Author

jedevc commented Oct 14, 2024

Tested that this works as desired using #153 (see https://github.com/jedevc/dagger-for-github/actions/runs/11331238930/job/31510817051).

Copy link
Member

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

Some of this logic is already implemented in install.sh as well as install.ps1, e.g.

  if [ -n "$DAGGER_COMMIT" ]; then
    path="main/${DAGGER_COMMIT}"
  elif [ -n "$DAGGER_VERSION" ]; then
    path="releases/${DAGGER_VERSION}"
  else
    path="releases/$(latest_version)"
  fi

Instead of adding new behaviour to the version property, I would add a new property - maybe build - and pass it through as DAGGER_COMMIT which already has higher precedence than DAGGER_VERSION.

If we did that, this would become a much smaller change. We could also test those functions in isolation, rather than needing to test this GitHub Action.

@jedevc
Copy link
Member Author

jedevc commented Oct 14, 2024

IMO it makes more sense to have a single combined field whenever possible - having multiple fields with precedence, etc, is confusing for end users.

Maybe we should put this logic into install.sh and install.ps1 and deprecate the old commit variable? That seems like a better interface than inheriting the separate variables that the install scripts do.

I also think we should add the tests regardless of what we do here - agreed it sucks not to be able to be able to test them outside of GitHub - but given the fragility of GitHub actions in general, having some sanity tests that just check it's working is still useful IMO. Especially if we go on to upstream more behavior from our custom "call" action in dagger/dagger.

@jedevc jedevc force-pushed the allow-specifying-nightly-commit branch from c524562 to 6e16225 Compare October 21, 2024 12:17
action.yml Outdated
Comment on lines 62 to 66
# If the dagger commit is 'latest', set the version back to 'head'.
COMMIT=${{ inputs.commit }}
if [[ "$VERSION" == "latest" ]]; then
VERSION="head"
fi
Copy link
Member

Choose a reason for hiding this comment

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

The best approach would be to pass both values as they are and trust install.sh to do the right thing.

head only makes sense in the context of COMMIT as in the HEAD commit for the main branch, i.e.

curl -I https://dl.dagger.io/dagger/main/head/checksums.txt
HTTP/2 200
content-type: text/plain; charset=utf-8
content-length: 1838
date: Tue, 22 Oct 2024 15:09:24 GMT
last-modified: Tue, 22 Oct 2024 12:46:28 GMT
etag: "8b336b45f49a98e101aaa43ca402ae32"
x-amz-server-side-encryption: AES256
content-disposition: attachment;filename=checksums.txt
accept-ranges: bytes
server: AmazonS3
x-cache: Miss from cloudfront
via: 1.1 01c1372965efe3974af81a7941e07b0c.cloudfront.net (CloudFront)
x-amz-cf-pop: LHR5-P7
alt-svc: h3=":443"; ma=86400
x-amz-cf-id: kYBZp0pDrxYjecpPfJFjnAgNmxUTFZUQhHPXBNQ3qZujDOmjLdPWKw==

latest only makes sense in the context of VERSION as in the latest version, i.e.

curl -I https://dl.dagger.io/dagger/versions/latest
HTTP/2 200
content-type: binary/octet-stream
content-length: 7
date: Tue, 22 Oct 2024 15:10:49 GMT
last-modified: Thu, 10 Oct 2024 22:27:36 GMT
etag: "818b6f96a7646a5e9e0ede3d9a27e78d"
x-amz-server-side-encryption: AES256
accept-ranges: bytes
server: AmazonS3
x-cache: Miss from cloudfront
via: 1.1 f5db953762dd20ad78d8b2c1e8d66550.cloudfront.net (CloudFront)
x-amz-cf-pop: LHR5-P7
alt-svc: h3=":443"; ma=86400
x-amz-cf-id: Dm3qHYxI8k6fqtsnIrBvV4ZuLGtrktmJWD6Bx_0y3DEveUt6ukbZpg==

Expected behaviour:

  1. version defaults to a specific version
  2. version can be set explicitly, e.g. 0.13.4
  3. commit is empty by default
  4. if commit is set, it takes precedence over version
  5. commit can be set to a specific git sha
  6. commit can be set to head

Copy link
Member Author

Choose a reason for hiding this comment

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

head only makes sense in the context of COMMIT as in the HEAD commit for the main branch, i.e.

Done.

The best approach would be to pass both values as they are and trust install.sh to do the right thing.

We can't do this yet - we need to update install.sh to handle "latest" if we want to do this.

$ curl -fsSL https://dl.dagger.io/dagger/install.sh | DAGGER_VERSION=latest sh
sh debug downloading files into /tmp/tmp.gWarmHq8ep
sh debug http_download https://dl.dagger.io/dagger/releases/latest/dagger_vlatest_linux_amd64.tar.gz
sh debug http_download_curl received HTTP status 403

@jedevc jedevc force-pushed the allow-specifying-nightly-commit branch 3 times, most recently from 08ab7ec to f562e6f Compare October 22, 2024 15:30
@jedevc jedevc force-pushed the allow-specifying-nightly-commit branch from f562e6f to 0ccd198 Compare October 22, 2024 15:32
Copy link
Member

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

LGTM!

@gerhard gerhard merged commit 49dd901 into dagger:main Oct 22, 2024
1 check passed
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.

4 participants