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

[CI] Exclude doc files from CI to save CI time #4691

Merged
merged 7 commits into from
Apr 20, 2021

Conversation

ueqri
Copy link
Contributor

@ueqri ueqri commented Apr 5, 2021

Added the exclude rules for push and PR triggers. So updates to doc, README, etc. won't cause the formatting + main CI to run.

Fixed PointCloudLibrary#3999. Updates to doc, README, etc. don't run formatting + main CI.
@kunaltyagi
Copy link
Member

This PR still runs the main CI for docs. Create a new issue for the docs or edit the existing issue to reflect the partially resolved state?

@ueqri
Copy link
Contributor Author

ueqri commented Apr 5, 2021

This PR still runs the main CI for docs. Create a new issue for the docs or edit the existing issue to reflect the partially resolved state?

yeah, current CI build is still based on the old azure pipelines file in master. It will take effect after merging this PR.
Could I fix some small typo in pcl_style_guide I've found before to reflect the partially resolved state?

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Can't rescind my review from mobile, hence this. Please ignore

Adding the rationale in a second review below

@ueqri
Copy link
Contributor Author

ueqri commented Apr 6, 2021

I've implemented the new pipeline in .ci/docs-pipeline.yaml. If everything is ok, please add the pipeline in the Azure DevOps after merging. Thank you!

@ueqri
Copy link
Contributor Author

ueqri commented Apr 9, 2021

I think the latest commit looks good now. Could you take a look again please?

@ueqri
Copy link
Contributor Author

ueqri commented Apr 10, 2021

I admit that the method is kind of obscure and tricky now. I'd add git check to make it more legible and comprehensible.

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Looks good.

@ueqri Could you do the following:

  • Create a dummy project to show the CI runs (I believe you've already done this)
  • Create a PR that runs only doc pipeline
  • Create a PR that runs build then doc pipeline

.ci/azure-pipelines/docs-pipeline.yaml Show resolved Hide resolved
@ueqri
Copy link
Contributor Author

ueqri commented Apr 10, 2021

Looks good.

@ueqri Could you do the following:

* Create a dummy project to show the CI runs (I believe you've already done this)

* Create a PR that runs only doc pipeline

* Create a PR that runs build then doc pipeline

Of course:satisfied:, I will do it in my repository and paste the result here soon. Thank you:satisfied:!

Those md files are not part of the documentation, so no need for trigger.
@ueqri
Copy link
Contributor Author

ueqri commented Apr 10, 2021

* Create a dummy project to show the CI runs (I believe you've already done this)

* Create a PR that runs only doc pipeline

* Create a PR that runs build then doc pipeline

I've made this in https://github.com/ueqri/pcl/tree/test-new-docs-pipeline, and related Azure pipelines dashboard are here: https://dev.azure.com/ueqri-ci/pcl-test/_build?view=runs

As I didn't set branch rules for triggers, some push beyond test-new-docs-pipeline branch also run CI and I canceled them immediately. So the result is a bit messy, I'm sorry for that.

The exactly related CI run are:

removed md files from PR trigger, and added conditions for `documentation` and `tutorials` stages to make sure they run as expected.
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

The sample links look good.

Just a few minor comments

.ci/azure-pipelines/docs-pipeline.yaml Show resolved Hide resolved
.ci/azure-pipelines/docs-pipeline.yaml Show resolved Hide resolved
@ueqri
Copy link
Contributor Author

ueqri commented Apr 16, 2021

I'm gratitude to you for helping me and guiding me in this PR. Thank you! 😀

And I was wondering if this could be merged, and then I could contribute to this issue #3947 based on the changes in current branch. It's truly a great pleasure:heart:!

@kunaltyagi kunaltyagi requested a review from larshg April 18, 2021 04:35
@kunaltyagi
Copy link
Member

Note to maintainers: We'll need to update the list of required CI for merge

@kunaltyagi kunaltyagi merged commit 2759717 into PointCloudLibrary:master Apr 20, 2021
@kunaltyagi
Copy link
Member

An error occurred while loading the YAML build pipeline. Unexpected symbol: ')'. Located at position 49 within expression: ne(variables['Build.Reason'], 'ResourceTrigger')). For more help, refer to https://go.microsoft.com/fwlink/?linkid=842996

@ueqri
Copy link
Contributor Author

ueqri commented Apr 20, 2021

I'm really sorry to bring this mistake in the last stage:cry:, and thanks for your hotfix. I promise I will check more carefully at any time especially in online code edits. I apologize for causing this trouble.

@kunaltyagi
Copy link
Member

Meh. Happens :)

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.

3 participants