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

feat: add dependency check step to build pipeline #16265

Merged
merged 8 commits into from
Dec 10, 2021

Conversation

tadelesh
Copy link
Member

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@tadelesh
Copy link
Member Author

tadelesh commented Nov 24, 2021

Incompatible dependency with different packages will cause mod auto-resolving failure when customer import multiple v2 packages including data plane and mgmt. plane SDK. This script added to build process will import all the v2 modules and try to resolve dependency as well as build and vet to ensure any update not breaking the package compatible. But it still exists a chicken and egg situation. This step will fail if any breaking change in common package like azcore has not applied to all packages. I have no perfect solution. So I just ignore the check for the first layer dependent common packages.

@jhendrixMSFT
Copy link
Member

This shouldn't be a problem post-GA. Seems expensive to build the world for each PR.

@tadelesh
Copy link
Member Author

tadelesh commented Nov 25, 2021

This shouldn't be a problem post-GA. Seems expensive to build the world for each PR.

According to ci log, it will take about 3.5mins in average. It seems expensive indeed, but I think maybe it is valuable before GA as we may have multiple breaking changes. I don't know whether there is a more economic way to do such check, maybe we could just use it temporary and keep it as a tool after GA. @lirenhe @ArcturusZhang what do you think?

@seankane-msft
Copy link
Member

/azp run go - azkeys

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@seankane-msft
Copy link
Member

/azp run go - azkeys

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@seankane-msft
Copy link
Member

@tadelesh in the tests run against azkeys, the go build command fails with exit code 1 but without any details on what the error is. Does this work locally for you?

@tadelesh
Copy link
Member Author

tadelesh commented Dec 3, 2021

/azp run go - azkeys

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tadelesh
Copy link
Member Author

tadelesh commented Dec 3, 2021

@tadelesh in the tests run against azkeys, the go build command fails with exit code 1 but without any details on what the error is. Does this work locally for you?

I've removed all the -x flag in go build cmd to give a more readable log. @seankane-msft You can get the error for azkeys for now.

@seankane-msft
Copy link
Member

I've removed all the -x flag in go build cmd to give a more readable log. @seankane-msft You can get the error for azkeys for now.

Thanks for removing that flag, the output is much clearer now. I have a fix for that once I can release changes to internal. For now that should not be blocking on this PR.

@tadelesh
Copy link
Member Author

tadelesh commented Dec 4, 2021

I've removed all the -x flag in go build cmd to give a more readable log. @seankane-msft You can get the error for azkeys for now.

Thanks for removing that flag, the output is much clearer now. I have a fix for that once I can release changes to internal. For now that should not be blocking on this PR.

@seankane-msft If this PR merge first, will it be conflict with the 'nightly build' change you are doing?

@benbp @weshaggard Could you help to have a review?

@seankane-msft
Copy link
Member

I've removed all the -x flag in go build cmd to give a more readable log. @seankane-msft You can get the error for azkeys for now.

Thanks for removing that flag, the output is much clearer now. I have a fix for that once I can release changes to internal. For now that should not be blocking on this PR.

@seankane-msft If this PR merge first, will it be conflict with the 'nightly build' change you are doing?

I don't believe so.

Copy link
Member

@seankane-msft seankane-msft left a comment

Choose a reason for hiding this comment

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

LGTM

@tadelesh
Copy link
Member Author

tadelesh commented Dec 7, 2021

/azp run go - azkeys

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tadelesh
Copy link
Member Author

tadelesh commented Dec 8, 2021

/azp run go - azkeys

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

A couple comments but looks good.

@tadelesh
Copy link
Member Author

/check-enforcer override

@tadelesh tadelesh merged commit b21a32a into Azure:main Dec 10, 2021
@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Dec 10, 2021

I'm unclear why this was merged, especially when we a) have breaking changes that haven't been released yet and b) have a few more on the way (you can see that CI failed in this PR even). I thought this was going to be enabled post-GA?

@benbp
Copy link
Member

benbp commented Dec 10, 2021

The check enforcer should not have been overridden here (at the very least without a comment on justification) when the pipeline failure was a direct result of the change.

@tadelesh
Copy link
Member Author

The check enforcer should not have been overridden here (at the very least without a comment on justification) when the pipeline failure was a direct result of the change.

Got it. This check is a chicken-and-egg conundrum. So I just bypassed it if any breaking change is intentional. It's good to be set as an optional before GA.

@tadelesh tadelesh deleted the dep_check_pipeline branch December 20, 2021 05:47
jhendrixMSFT pushed a commit to jhendrixMSFT/azure-sdk-for-go that referenced this pull request Jan 12, 2022
* feat: add dependency check step to build pipeline

* fix: move depency check from build-test to analyze

* fix: remove all -x flag in go build to show clear error log

* fix: change param path and simplify implementation

* fix: remove unnecessary ignore package

* fix: use retract info to judge whether a package is deprecated

* fix: change path compare to more simple way

* fix: some script refine
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