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

Publish sources of examples as separated artifacts are attached to pre-release #569

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

ihostage
Copy link
Member

@ihostage ihostage commented Mar 4, 2024

Advantages:

  1. Permanent link for any example, that can be use in documentation
  2. Filter build system files (Sbt/Gradle) for downloadable example
  3. Simplify playing with examples for users, instead of clone a whole repository and jump to folder of interesting example

How it works, see:
https://github.com/ihostage/play-samples/releases/tag/nightly

@ihostage ihostage requested a review from mkurz March 4, 2024 13:19
@ihostage ihostage mentioned this pull request Mar 4, 2024
Copy link
Member

@mkurz mkurz left a comment

Choose a reason for hiding this comment

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

👍 I like the idea.

We can merge that, however I am pretty sure the sources.sh script can be made much shorter, by putting the examples in an array and then iterating over it.
Something like this (pseudo code! does not work!):

#!/usr/bin/env bash

samples="play-java-dagger2", "play-java-dagger2",...

for sample in samples
  pushd ${sample}-example
  zip -r -q ../${sample}-sbt-example.zip . -x "gradle/*" -x "scripts/*" -x "*gradle*"
  zip -r -q ../${sample}-gradle-example.zip . -x "project/*" -x "scripts/*" -x "*.sbt"
  popd
end for

Even more, instead of hardcoding an array you could even just list all directories and use that, something like:

...
samples=$(ls -d */ | cut -f1 -d'/') # from https://stackoverflow.com/a/22072749/810109
...

This way we don't have to include update the script each time (but we have to for the .yml file...)

We can optimize this later of course and merge this for now.

@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

We can merge that, however I am pretty sure the sources.sh script can be made much shorter, by putting the examples in an array and then iterating over it.

Yes, of course I had thought about that, but decided to stay a verbose variant before finilize #558 😄
I think we can optimize this script when we'll have a new structure

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

but we have to for the .yml file..

Actually I think we can also just use *-example.zip (or similiar) for the files input in the yml file! Look at https://github.com/softprops/action-gh-release?tab=readme-ov-file#%EF%B8%8F-uploading-release-assets:

Use the with.files input to declare a newline-delimited list of glob expressions matching the files you wish to upload to GitHub releases. If you'd like you can just list the files by name directly.

It says you can use glob expressions, so that should avoid listing all zip files per hand.

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

👍 I am ok to merge this if we know if the tag gets overriden or not, or if we have to delete it before creating a new one.

@mkurz mkurz mentioned this pull request Mar 4, 2024
3 tasks
@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

Created

@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

👍 I am ok to merge this if we know if the tag gets overriden or not, or if we have to delete it before creating a new one.

I published examples twice for my fork (https://github.com/ihostage/play-samples/actions) that's why I'm sure we don't need to delete something manually before update artifacts. Working process shouldn't change, we just still merge PRs into target branches and published artifacts will be updated and can be re-downloaded by the same links.

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

I published examples twice for my fork (https://github.com/ihostage/play-samples/actions) that's why I'm sure we don't need to delete something manually before update artifacts. Working process shouldn't change, we just still merge PRs into target branches and published artifacts will be updated and can be re-downloaded by the same links.

I understand, I was just wondering if the nightly tag actually changes its sha position. Like after merging a PR I understand the zip files will be overriden. But will the nightly tag be moved to latest HEAD (main branch in our case) then? Or will the tag stay at the same commit like before?
Actually, thinking about it, it is not so important if the tag does not move, it would be just nice maybe. If not its probably still ok.

@ihostage ihostage force-pushed the publish-sources branch 2 times, most recently from 453caf7 to 2b1a3a7 Compare March 4, 2024 14:09
@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

I understand, I was just wondering if the nightly tag actually changes its sha position. Like after merging a PR I understand the zip files will be overriden. But will the nightly tag be moved to latest HEAD (main branch in our case) then? Or will the tag stay at the same commit like before? Actually, thinking about it, it is not so important if the tag does not move, it would be just nice maybe. If not its probably still ok.

Unfortunately, tag doesn't update only artifacts, see softprops/action-gh-release#293
But we can use a WA and delete release and tag before publish by https://github.com/dev-drprasad/delete-tag-and-release (see this comment softprops/action-gh-release#45 (comment))

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

Unfortunately, tag doesn't update only artifacts, see softprops/action-gh-release#293

OK, so the artifacts get updated, but the tag not.

But we can use a WA and delete release and tag before publish by https://github.com/dev-drprasad/delete-tag-and-release (see this comment softprops/action-gh-release#45 (comment))

I think it's not necessary to do this workaround, let's just wait for

I think it is not so important to move the tag if the artifacts are up to date anyway. Also the link (https://github.com/playframework/play-samples/releases/tag/nightly) does not change so this is also ok.
WDYT?

@ihostage ihostage force-pushed the publish-sources branch 3 times, most recently from 80d4b08 to 951dda6 Compare March 4, 2024 14:35
@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

So...
✅ I added step with delete previous tag and checked it in my fork. Works fine
✅ Change with.files to glob expressions as you suggested, thank you 👍
✅ Set target_commitish as ${{ github.ref_name }}. It should work fine for any branches not only for main

I think we are ready to merge 🏃‍♂️

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

👍

@mkurz mkurz merged commit 73d018b into playframework:main Mar 4, 2024
17 checks passed
@ihostage ihostage deleted the publish-sources branch March 4, 2024 15:06
@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

@Mergifyio backport 2.9.x

@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

@Mergifyio backport 3.0.x

Copy link
Contributor

mergify bot commented Mar 4, 2024

backport 2.9.x

✅ Backports have been created

Copy link
Contributor

mergify bot commented Mar 4, 2024

backport 3.0.x

✅ Backports have been created

@mkurz
Copy link
Member

mkurz commented Mar 4, 2024

Actually thinking about it... the name nightly is not 100% technically correct, because it is not a nightly build (we do not build the samples zip's every night, which also is not necessary, but "just" on each push).
I mean, this might be a bit nitpicking now, but probably a name/tag like dev, development, unstable, next, etc. etc. would probably fit a bit better. But we can keep nightly, I mean, it's not sooo important and the result is the same anyway eventually.

mkurz added a commit that referenced this pull request Mar 4, 2024
[2.9.x] Publish sources of examples as separated artifacts are attached to pre-release (backport #569) by @ihostage
mkurz added a commit that referenced this pull request Mar 4, 2024
[3.0.x] Publish sources of examples as separated artifacts are attached to pre-release (backport #569) by @ihostage
@ihostage
Copy link
Member Author

ihostage commented Mar 4, 2024

@mkurz Yep, I agree that nightly doesn't a correct, I just copied it from one example and wanted to discuss this name with you. I think we can change it in working on #570 after some thinking times 😉

@mkurz
Copy link
Member

mkurz commented Jul 30, 2024

Seems like

is done if it still helps.

@ihostage
Copy link
Member Author

Hmmm... I see only one difference between the recommendation (softprops/action-gh-release#171 (comment))

- name: Update dev-build tag
  if: ${{ github.event_name == 'push' }}
  run: |
    git tag -d ${{ env.dev_tag }} || true
    git push origin :refs/tags/${{ env.dev_tag }} || true
    git tag ${{ env.dev_tag }}
    git push origin ${{ env.dev_tag }}

and our implementation

- name: Delete Release
  run: gh release delete nightly --cleanup-tag || true
  env:
    GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

I think we don't need change anything 🤔

@mkurz
Copy link
Member

mkurz commented Jul 30, 2024

I think we don't need change anything 🤔

OK 😉

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.

2 participants