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

Add docker image #192

Merged
merged 5 commits into from
Dec 5, 2021
Merged

Conversation

davideicardi
Copy link
Contributor

Fix #191

@davideicardi
Copy link
Contributor Author

I have tested it locally but not sure if github action script is correct ... Not sure how to test it.

@davideicardi
Copy link
Contributor Author

And you have to set DOCKER_TOKEN secret I suppose.

Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
@Anthony-Mckale
Copy link

really nice guessing once this is approved owner can create

https://hub.docker.com/r/cyclonedx/cyclonedx-node

to match https://hub.docker.com/r/cyclonedx/cyclonedx-python

@Anthony-Mckale
Copy link

Anthony-Mckale commented Nov 10, 2021

would perhaps add a "docker-release" command to the scripts but that's just me being lazy for the project i maintain

something like

"scripts": {
...
"docker-release": "docker build . -t cyclonedx/cyclonedx-node:`node bin/cyclonedx-bom --version` -t cyclonedx/cyclonedx-node:latest && docker push cyclonedx/cyclonedx-node:`node bin/cyclonedx-bom --version` && docker push cyclonedx/cyclonedx-node:latest"
}

Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
@davideicardi
Copy link
Contributor Author

Thank you @Anthony-Mckale. Just added docker-release script as suggested.

@davideicardi
Copy link
Contributor Author

News? It is possible to publish the official docker image? Thank you!

@jkowalleck
Copy link
Member

@coderpatros @stevespringett your attention is needed

Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
run: |
echo Version level: ${{ github.event.inputs.versionLevel }}
npm ci
npm run build --if-present
npm version ${{ github.event.inputs.versionLevel }} --message "${{ github.event.inputs.commitMessage }}"
VERSION=$(npm version ${{ github.event.inputs.versionLevel }} --message "${{ github.event.inputs.commitMessage }}")
VERSION=${VERSION:1} # remove 'v' prefix
Copy link
Member

@jkowalleck jkowalleck Dec 4, 2021

Choose a reason for hiding this comment

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

dont modify the version.

as far as i know we like to have all versions the same - the node package and the docker.
if one has a v prefix, then all have it.

are there any needs to change this? does a leading v in docker tags break a workflow or are considered "bad habbit" ? please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is just a convention, usually docker tags doesn't have the v prefix. Also other CycloneDx images follow the same rules (https://hub.docker.com/r/cyclonedx/cyclonedx-python/tags, https://hub.docker.com/r/cyclonedx/cyclonedx-dotnet/tags, ...). Personally I prefer to remove the v.

But I understand your point so we can do whatever you prefer ;-).

Please let me know if I should leave the v prefix or not.

COPY . /usr/src/cyclonedx-bom

ENTRYPOINT ["/usr/src/cyclonedx-bom/bin/cyclonedx-bom"]
CMD ["-h"]
Copy link
Member

Choose a reason for hiding this comment

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

why add a default command?

cyclonedx-bom can run without any arguments/options/switches.
having this default option -h would force the user to add at least one arguments/options/switches
am i wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right now if you do not add any arguments it will just show the help.

In my opinion is a good practice to avoid unexpected behavior and it is the same logic used by cyclonedx-dotnet.

But also in this case it is not a strong opinion, as you prefer.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jkowalleck
Copy link
Member

thanks again, @davideicardi
your effort is appreciated.

I've added some remarks in the files, that you need to consider.
Additionally, could you also add a docker watcher for dependabot to .github/dependabot.yml ?

Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
Signed-off-by: Davide Icardi <davide.icardi@gmail.com>
@davideicardi
Copy link
Contributor Author

@jkowalleck Thank you for your feedbacks.

I have added dependabot and fixed or answered to your points.

Happy to contribute to cyclonedx ecosystem!

@stevespringett
Copy link
Member

Big thanks for this PR and for all the feedback. Much appreciated.

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.

Can we create an official docker image?
4 participants