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

Guides Gitlab update .gitlab-ci.yml example #5294

Merged

Conversation

Ethirallan
Copy link
Contributor

Description (required)

This commit addresses a syntax error in the .gitlab-ci.yml file that was causing the pipeline to fail. The error was due to incorrect indentation in the YAML file.

Additionally, the before_script block, which was previously defined globally, has been moved inside the pages job. This change provides better flexibility for users who have multiple stages in their pipelines, as the before_script will now only run before the pages job, rather than before every job in the pipeline.

These changes should resolve the pipeline failure and prevent potential issues in multi-stage pipelines.

This commit addresses a syntax error in the .gitlab-ci.yml file that was causing the pipeline to fail.
The error was due to incorrect indentation in the YAML file.

Additionally, the `before_script` block, which was previously defined globally, has been moved inside
the `pages` job. This change provides better flexibility for users who have multiple stages in their
pipelines, as the `before_script` will now only run before the `pages` job, rather than before every
job in the pipeline.

These changes should resolve the pipeline failure and prevent potential issues in multi-stage pipelines.
Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 809b248
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/654a238ea801160008cb8397
😎 Deploy Preview https://deploy-preview-5294--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@astrobot-houston astrobot-houston left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for opening your first PR to Astro’s Docs! 🎉

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any broken links you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Netlify 🥳

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

Hello @Ethirallan, thank you for handling this and proposing these changes. I'm just wondering if you tested this in prod, cause when I was playing around with gitlab I didn't face any issues with the current script. I agree with moving the before_script inside the pages block making the before_script only apply to the pages job rather than all jobs.

@Ethirallan
Copy link
Contributor Author

Hello @dreyfus92, thank you for a fast reply. We just started migrating our docs to Astro (starlight) and successfully deployed the first one on Friday. Today we had problems with the second one (copied again pipeline from the docs) and had a failed pipeline because of the invalid yaml syntax: image, before_script and pages stage were not indented on the same level. You can double check this with yaml validator. It was an easy fix and after testing with gitlab-runner I prepared this PR.

On the other hand, I'm now thinking about if it would be a good idea to move image inside pages stage as well. This way the whole snippet can me easily copied to any exiting pipeline as well as used as a standalone one. What is you opinion on this?

@dreyfus92
Copy link
Member

Thanks for the additional context about starlight @Ethirallan . I appreciate you taking the time to explain the full background and proposal.

You're absolutely right that the YAML indentation was invalid in the original - the image, before_script, and pages all needed to be aligned under stages for it to be valid syntax. I'm glad the fix resolved the pipeline failure.

Regarding moving image under the pages stage, I think that's a great idea for improving reusability as you mentioned. Defining the runner image only once where it's needed makes the config more modular. People can easily copy just the pages stage to add Astro building to an existing pipeline, without having to define the image globally.

The only consideration is if you have any other stages in the pipeline that may also depend on that image. But for a simple Astro docs pipeline focused just on building pages, consolidating everything into the one pages stage makes total sense.

Overall I think that change would provide a cleaner, more portable Astro config snippet that can drop easily into other pipelines. Feel free to update the PR if you'd like 😁.

I'd also like some input from @delucis and @sarah11918 to see what are their thoughts regarding this matter 😊

@Ethirallan
Copy link
Contributor Author

You are welcome and thank you for this amazing feedback @dreyfus92 . Your detailed explanation of all points is greatly appreciated 😁.

Regarding the consideration you have a good point, but I think this is beyond the scope of Astro docs and depends from use case to use case. In my opinion the provided snippet should be designed to function as a standalone pipeline initially (as it already is in the published docs). If it can be consolidated (like in this case) it is just a nice bonus and it would serve as a good starting point for integration into existing and more complex pipelines. By this stage, users are likely equipped with the knowledge to optimize their solutions accordingly anyway.

I'll wait for further feedback and update this PR according afterwards 😄.

@sarah11918
Copy link
Member

Thanks everyone for this lovely discussion!

@Ethirallan We really appreciate you taking the time to correct the docs with what worked for you, and helping us to have a working, standalone code sample.

Since @dreyfus92 has the most experience with this workflow, I am happy to let him make the final call. When I see that he has approved this PR, I will consider it ready to merge!

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and discussion! This also looks good to me, although quick note that currently as well as adjusting the YAML indentation, this PR reduces the indentation of the entire codeblock from 3 to 2 spaces. This means Markdown no longer treats it as part of the numbered item above:

Current Docs This PR
image image

It’s a subtle difference (look at how the code block’s left edge aligns with the text above it) but we should make sure to restore the three-space indentation before merging (while also making sure the resulting YAML indentation is correct).

…ent fix

In previus commit the whole code block with pipeline example was moved.
This commit fixes the indentation issue and also moves image inside pages
for better reusability.
@Ethirallan
Copy link
Contributor Author

Thank you for your feedback, @sarah11918 and @delucis, really nice catch with wrong codeblock indentation. I have made the following changes based on all comments:

  • Fixed the codeblock indentation issue
  • Moved image inside pages for better reusability
  • Tested the changes with gitlab-runner and in one of our projects

If you have any further comments or suggestions, please let me know and I will be happy to make the necessary changes 😄.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Fantastic — thanks for the thorough testing @Ethirallan!

@sarah11918 sarah11918 merged commit 32152cb into withastro:main Nov 7, 2023
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.

5 participants