search

ACCESS-NRI / access-hive.org.au

Documentation hub for the Earth System models, ACCESS, and their community
https://access-hive.org.au/
8 stars 11 forks source link

Weekly merge #629

Closed atteggiani closed 9 months ago

atteggiani commented 9 months ago

Weekly merge after sprint meeting

KAUR1984 commented 9 months ago

Thanks @atteggiani! CI seems to be failing currently... Was that something you noticed already?

atteggiani commented 9 months ago

Thanks @atteggiani! CI seems to be failing currently... Was that something you noticed already?

Yeah I noticed that, but it's just the Markdown links check that fails (still not sure why. I checked the error and the links that it shows as failing seem to be properly working for me..). So I would disregard that for now.

flicj191 commented 9 months ago

Thanks @atteggiani! CI seems to be failing currently... Was that something you noticed already?

Yeah I noticed that, but it's just the Markdown links check that fails (still not sure why. I checked the error and the links that it shows as failing seem to be properly working for me..). So I would disregard that for now.

I think the checks doesn't like a particular link style or relative path?: image eg. ln 32: ... general prerequisites outlined in the [First Steps](/getting_started/first_steps) section. could be similar to html style then it doesn't check it in ln 61: If you are not familiar with ARE, check out the <a href="/getting_started/are">Getting Started on ARE</a> section.

I'll try it in a branch and create pr

atteggiani commented 9 months ago

@flicj191 Those links are absolute paths. That's markdown language for links. It is present in other pages and the "check markdown links" works well with them. I don't know why these are failing.

Your solution works of course because you change the language from markdown to HTML, so the "check markdown links" does not capture them anymore, but still does not explain why the error gets raised in the first place.

However, in general I prefer to have links in HTML style, especially because you have a lot more flexibility with them (you can add classes, you can open them in another tab, etc.) so I am pretty happy with changing them to HTML.

I think it's also worth having a look for GitHub actions that can check HTML links the same way the markdown ones are checked (basically they raise an error if they encounter a 404 response), so we can potentially replace all markdown links in the website with HTML links but still have a way to have a minimum check that they are not broken.

KAUR1984 commented 9 months ago

Thanks @atteggiani ! I think we should avoid merging anything into the main branch unless the CI is passing... I have read your above points and they all look pretty valid to me :). I am happy to help with resolving this CI issue and perhaps we could discuss it together afterwards..

atteggiani commented 9 months ago

Thanks @atteggiani ! I think we should avoid merging anything into the main branch unless the CI is passing... I have read your above points and they all look pretty valid to me :). I am happy to help with resolving this CI issue and perhaps we could discuss it together afterwards..

I agree in general we should avoid merging things when CI is failing, but there might be exceptions, especially regarding the link check. If that is failing but we don't see problems with the failing links in deployment, it's safe to merge anyway. What we should do is definitely find out the cause of the issue so we don't have (or at least limit) falsely-detected broken links.

Anyway the issue is now solved as @flicj191 already changed the links' structure to HTML in another PR, in fact now all CI/CD workflows pass.

As I said above, what is useful now I think it's find an existing action that is able to detect HTML broken links, so we can safely change the structure of all the links on the websites from markdown to HTML.

Also, regarding this PR, feel free to approve and merge.