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

add Woodpecker #1880

Merged
merged 20 commits into from
Jul 24, 2022
Merged

add Woodpecker #1880

merged 20 commits into from
Jul 24, 2022

Conversation

uncomfyhalomacro
Copy link
Contributor

@uncomfyhalomacro uncomfyhalomacro commented Jul 19, 2022

Draft for now since I am going to add some documentation first :D

Edit by @mortenpi: close #1878

@uncomfyhalomacro
Copy link
Contributor Author

image
Currently works during my experiment on adding it

src/deployconfig.jl Outdated Show resolved Hide resolved
@uncomfyhalomacro uncomfyhalomacro marked this pull request as ready for review July 19, 2022 10:00
@uncomfyhalomacro
Copy link
Contributor Author

uncomfyhalomacro commented Jul 19, 2022

I still have to figure out autodetection since CI is commonly used as an env var for all forges such as Github
I realized i can just add it as last since CI may return woodpecker or drone

@uncomfyhalomacro
Copy link
Contributor Author

@mortenpi @fredrikekre fixed a lot of typos now that i missed and i rebased it so and forced push with lease to make the history clean. Works fine now on my project that i tested this branch on.

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Please use 4 space indent. Can you also add a note about this in the CHANGELOG.md file?

src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
@uncomfyhalomacro uncomfyhalomacro force-pushed the woodpecker branch 2 times, most recently from 90c8a4e to d5740f1 Compare July 19, 2022 11:40
@uncomfyhalomacro
Copy link
Contributor Author

This should be ready for another review now 😄

@fredrikekre
Copy link
Member

Looks fine, can you add some tests to https://github.com/JuliaDocs/Documenter.jl/blob/master/test/deployconfig.jl?

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Looks great for the most part, just some nitpicks.

  • Do you have an example run on Codeberg/Woopecker that uses this branch?
  • I don't think the PR is up to date with master? There should be some conflicts in the CHANGELOG.

src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
@uncomfyhalomacro
Copy link
Contributor Author

Looks great for the most part, just some nitpicks.

* Do you have an example run on Codeberg/Woopecker that uses this branch?

* I don't think the PR is up to date with master? There should be some conflicts in the CHANGELOG.

https://codeberg.org/uncomfyhalomacro/SBOL3.jl/src/branch/main/.woodpecker/.doc-woodpecker-experiment.yml I experimented this on my soon-ish to start project of mine. Result will update my pages branch https://codeberg.org/uncomfyhalomacro/SBOL3.jl/src/branch/pages. -> the result would be this https://uncomfyhalomacro.codeberg.page/SBOL3.jl/dev/

I will rebase from upstream branch then :D and fix some conflicts

@uncomfyhalomacro
Copy link
Contributor Author

Just found an issue on the rendered pages where if I click the edit button, it gives the wrong link even with correct root URL. I have to fix and resolve results from the code review later.

@mortenpi
Copy link
Member

If the edit link behavior was something that just appeared, then it might also be something from #1808.

@uncomfyhalomacro
Copy link
Contributor Author

If the edit link behavior was something that just appeared, then it might also be something from #1808.

nah i found out it was something else haha. anyway. it is ready for review now (again) :D

@mortenpi
Copy link
Member

it is ready for review now (again) :D

Did you forget to push? I see you marked the comments resolved, but I believe the code hasn't changed?

@uncomfyhalomacro
Copy link
Contributor Author

uncomfyhalomacro commented Jul 21, 2022

it is ready for review now (again) :D

Did you forget to push? I see you marked the comments resolved, but I believe the code hasn't changed?

No, it has changed. I just need to push one more to resolve authenticated_repo_url.

EDIT: resolved in commit bc81ef6

Soc Virnyl S. Estela added 6 commits July 21, 2022 09:55
- PR should use built-in env vars `CI_COMMIT_REF` and `CI_COMMIT_PULL_REQUEST`
- GitHub Actions and Woodpecker share similarities thus modified some stuff slightly.
- `FORGE_URL` should be not a link but the root of a URL e.g. github.com, codeberg.org
- `CI_REPO_LINK` should be used if `FORGE_URL` is not defined.
- return an empty string if regex returns no match (which
  is a value of `nothing`. The error will be caught
  anyway in `deploydocs`
- handle `authenticated_repo_url` for woodpecker:
  * added more stuff into the `Woodpecker constructor`:
    - woodpecker_repo_link
    - woodpecker_forge_url
    - new constructor params are validated in `deploy_folder`

- improve docstring for completeness
- update Woodpecker constructor:
  - `CI_COMMIT_TAG` added to constructor
  - reducing logic if checking tag build is a valid version using
    `woodpecker_tag`

- improve test
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Cool! I think this LGTM now, other than the two minor things.

Do you links to a run on Codeberg with the latest version of this here by any chance, including the CI logs?

CHANGELOG.md Outdated Show resolved Hide resolved
src/deployconfig.jl Outdated Show resolved Hide resolved
Soc Virnyl S. Estela and others added 2 commits July 22, 2022 08:22
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@uncomfyhalomacro
Copy link
Contributor Author

uncomfyhalomacro commented Jul 22, 2022

Cool! I think this LGTM now, other than the two minor things.

Do you links to a run on Codeberg with the latest version of this here by any chance, including the CI logs?

https://ci.codeberg.org/uncomfyhalomacro/SBOL3.jl/build/12/1 here 😄

Configuration used:

matrix:
  JULIA_VERSION:
    - 1.7.3-alpine
  platform:
    - linux/amd64
platform: ${platform}
pipeline:
  build:
    when:
      branch:
        - main
    image: julia:${JULIA_VERSION}
    commands:
      - apk add git
      - julia -e 'using Pkg; Pkg.develop(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl");'
      - rm docs/Project.toml
      - julia --project=docs/ -e 'using Pkg; Pkg.add(url="https://github.com/uncomfyhalomacro/Documenter.jl", rev="woodpecker"); Pkg.add(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl")'
      - julia --project=docs/ docs/make.jl
    secrets: [ project_access_token ]

@mortenpi
Copy link
Member

Hmm. In the configuration, unless I am mistaken, but:

  • julia -e 'using Pkg; Pkg.develop(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl");' will just add SBOL3 into the global environment, which seems unnecessary?
  • The rm docs/Project.toml seems unnecessary -- you can just modify the existing environment. In fact, usually you'd want to just add current package as dev dep and then Pkg.instantiate it, especially if you have a bunch of other packages there.
  • Pkg.add(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl") will make the documentation build use your main version no matter what. You should do Pkg.develop(path=pwd()) instead, to make sure you're using SBOL3 from the checked out repository.

CHANGELOG.md Show resolved Hide resolved
@uncomfyhalomacro
Copy link
Contributor Author

uncomfyhalomacro commented Jul 22, 2022

Hmm. In the configuration, unless I am mistaken, but:

* `julia -e 'using Pkg; Pkg.develop(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl");'` will just add SBOL3 into the global environment, which seems unnecessary?

* The `rm docs/Project.toml` seems unnecessary -- you can just modify the existing environment. In fact, usually you'd want to just add current package as dev dep and then `Pkg.instantiate` it, especially if you have a bunch of other packages there.

* `Pkg.add(url="https://codeberg.org/uncomfyhalomacro/SBOL3.jl")` will make the documentation build use your `main` version no matter what. You should do `Pkg.develop(path=pwd())` instead, to make sure you're using SBOL3 from the checked out repository.

For the first one. Yeah I can remove it because that is just an experimentation for finding some issues from Pkg. It was because I have many errors caused by Pkg.jl. This is because the package is not yet registered so even with julia --project=docs/ using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate() that will error. So that is my workaround for now.

As for the second one, i have to remove it otherwise, it will instantiate an non-existing UUID (not registered). So removing Project.toml then adding the packages manually fixes my issue.

Third one, I think i stated the reason in the first one.

@uncomfyhalomacro
Copy link
Contributor Author

uncomfyhalomacro commented Jul 22, 2022

It is a separate issue. Maybe I should have open an issue in Pkg.jl hmm

Logs for why i use Pkg.add instead of the usual Pkg.develop(PackageSpec(path=pwd())): https://ci.codeberg.org/uncomfyhalomacro/SBOL3.jl/build/2/9

If you have a registered package, it should be fine.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
@mortenpi
Copy link
Member

mortenpi commented Jul 22, 2022

Logs for why i use Pkg.add instead of the usual Pkg.develop(PackageSpec(path=pwd())): https://ci.codeberg.org/uncomfyhalomacro/SBOL3.jl/build/2/9

If you put the add after the develop it should be fine I believe. The standard setup definitely works for unregistered packages.

@uncomfyhalomacro
Copy link
Contributor Author

Logs for why i use Pkg.add instead of the usual Pkg.develop(PackageSpec(path=pwd())): https://ci.codeberg.org/uncomfyhalomacro/SBOL3.jl/build/2/9

If you put the add after the develop it should be fine I believe. The standard setup definitely works for unregistered packages.

Going to try that

@uncomfyhalomacro
Copy link
Contributor Author

@mortenpi well that worked. thanks! https://ci.codeberg.org/SynBioJulia/SBOL3.jl/build/1/3 anyway just ignore the part where Documenter fails. Forgot to update my repository link. I moved my project to an org.

docs/src/man/hosting.md Outdated Show resolved Hide resolved
src/deployconfig.jl Show resolved Hide resolved
@uncomfyhalomacro
Copy link
Contributor Author

One more documentation on how to generate SSH keys and converting the private key to a base64 key and then another review so this could be merged

- also added a photo for a sample gitea repo
- also added the manual way if not using julia to generate base64-encoded keys using `openssl` and `tr`
@uncomfyhalomacro
Copy link
Contributor Author

@fredrikekre @mortenpi This is ready to be merged. You can review again I guess.

- add check if DOCUMENTER_KEY exists in woodpecker
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

@uncomfyhalomacro Would you be okay using the master branch of Documenter for a while? It's going to be a while before we tag 0.28.0, but I am not sure I want to go through the hassle of backporting this unless it's really necessary.

@fredrikekre fredrikekre merged commit d7861d1 into JuliaDocs:master Jul 24, 2022
@uncomfyhalomacro
Copy link
Contributor Author

It should be fine! No worries 😄

@mortenpi
Copy link
Member

Cool. And thanks a lot for iterating on the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Woodpecker in deployments
3 participants