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

Weekly merge #629

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Weekly merge #629

merged 8 commits into from
Dec 8, 2023

Conversation

atteggiani
Copy link
Contributor

Weekly merge after sprint meeting

@KAUR1984
Copy link
Contributor

KAUR1984 commented Dec 7, 2023

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

@atteggiani
Copy link
Contributor Author

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
Copy link
Contributor

flicj191 commented Dec 7, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor

KAUR1984 commented Dec 7, 2023

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
Copy link
Contributor Author

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.

@atteggiani atteggiani merged commit b265f57 into main Dec 8, 2023
9 of 10 checks 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